feat(transfer): hybrid rule-based + VDB semantic routing (closes #23)#90
Merged
feat(transfer): hybrid rule-based + VDB semantic routing (closes #23)#90
Conversation
…sue #23) Adds VDBRoutingConfig struct with confidence_threshold, min_samples_per_repo, max_candidates, and explain_decision fields, plus a Strategy field to TransferConfig. Includes defaults in applyDefaults() and mergeConfigs(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements VDBRouter.SuggestTransfer() which embeds the issue, searches the VDB collection, analyses repo distribution of results and returns a VDBMatchResult when one repo exceeds the confidence threshold with enough samples. Includes full unit-test coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds ExplainTransferInput type, LLMClient.ExplainTransfer() method (temperature 0.3), and buildExplainTransferPrompt() that generates a 2-3 sentence explanation of why an issue should be transferred. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TransferCheck now accepts embedder, vectorStore, and llmClient deps from the pipeline registry. After rule-based matching fails, when strategy is "hybrid" or "vdb-only" it calls VDBRouter.SuggestTransfer(). On a confident match it sets TransferTarget + metadata (transfer_method, transfer_confidence, transfer_reasoning). Optionally calls LLMClient.ExplainTransfer when explain_decision=true. Loop prevention and transfer_blocked flag still apply to VDB path. Includes integration tests for rule-priority, rules-only strategy, blocked flag and loop prevention. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements hybrid transfer routing that combines existing rule-based matching with VDB semantic search. It introduces a new VDBRouter component for semantic issue similarity analysis, extends TransferCheck to support multiple routing strategies, adds LLM-based explanation of transfer decisions, and updates configuration to support strategy selection and VDB routing parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Issue Pipeline
participant TC as TransferCheck
participant RuleMatcher as Rule Matcher
participant VDB as VDB Router
participant Embedder as Embedder
participant VectorStore as Vector Store
participant LLM as LLM Client
Client->>TC: Run(issue)
TC->>RuleMatcher: Check rules first
alt Rule Match
RuleMatcher->>TC: Return matching rule
TC->>Client: Set transfer_target, method=rule
else No Rule Match & Strategy=hybrid
TC->>VDB: SuggestTransfer(issue)
VDB->>Embedder: Embed(issue.title+body)
Embedder->>VDB: Return embedding vector
VDB->>VectorStore: Search(embedding, filters)
VectorStore->>VDB: Return similar issues with metadata
VDB->>VDB: Aggregate by repo, compute confidence
alt Confidence > Threshold
VDB->>TC: Return VDBMatchResult
TC->>LLM: ExplainTransfer(matches)
LLM->>TC: Return reasoning text
TC->>Client: Set transfer_target, method=vdb, reasoning
else Confidence Below Threshold
VDB->>TC: Return nil
TC->>Client: No transfer
end
else Strategy=rules-only or VDB disabled
TC->>Client: No transfer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (1)
internal/transfer/vdb_router_test.go (1)
159-179: Add a regression test for non-positivemaxCandidates.Given the slicing boundary path, add a case for
maxCandidates < 0(and optionally= 0) to lock in panic-free behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/transfer/vdb_router_test.go` around lines 159 - 179, Update the TestVDBRouter_MaxCandidatesTruncation test to include cases where maxCandidates is non-positive (e.g., 0 and a negative value) and assert SuggestTransfer on router (the newTestRouter instance) does not panic and returns either a nil suggestion or an empty SimilarIssues slice; specifically exercise router.SuggestTransfer(context.Background(), issueInput("foo","bar"), "org/current", 0.5, 1, 0) and with a negative maxCandidates to ensure the slicing boundary in SuggestTransfer is handled safely and no panic occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/config/config.go`:
- Around line 116-122: VDBRoutingConfig's ExplainDecision is a plain bool so
"unset" and false are indistinguishable and the merge logic (which currently
only copies true) prevents children from explicitly disabling explanations;
change ExplainDecision to *bool in VDBRoutingConfig (and mirror this pointer
change for the other affected bools mentioned), update the config
merge/merge-from logic to treat nil as "use default" (default true) and to copy
explicit false values from child configs, and ensure any code reading
ExplainDecision dereferences the pointer with a default true when nil.
In `@internal/steps/transfer_check_test.go`:
- Around line 104-146: The test TestTransferCheck_VDBFallback is actually
asserting the skip-path when embedder is nil, not VDB fallback; either rename
the test to reflect that behavior (e.g.,
TestTransferCheck_SkipsVDBWhenEmbedderNil) or implement a real fallback test by
making TransferCheck accept a mockable embedder: inject a shim embedder into the
TransferCheck (instead of embedder: nil) and ensure transfer.NewVDBRouter is
created with that mock so tcMockStore vdbResults are used; then run
step.Run(makeCtx(...)) and assert pipeCtx.TransferTarget is set to the expected
repo ("myorg/other-repo"). Ensure you update references to TransferCheck,
NewVDBRouter, tcMockStore, makeCtx, Run and TransferTarget accordingly.
In `@internal/steps/transfer_check.go`:
- Around line 23-26: Replace the concrete Gemini types with small local
interfaces to allow mocking: define an Embedder interface (e.g., with the
methods used from gemini.Embedder) and an LLMClient interface (with the methods
used from *gemini.LLMClient*), update the struct fields embedder and llmClient
in transfer_check.go to use these interfaces, and adapt any constructors or
initializers to accept/assign the concrete gemini implementations to those
interfaces; ensure the concrete gemini.Embedder and gemini.LLMClient still
satisfy the new interfaces so runtime behavior is unchanged and unit tests can
inject mocks.
In `@internal/transfer/vdb_router.go`:
- Around line 129-130: The slice operation ids = ids[:maxCandidates] can panic
if maxCandidates is negative; before slicing (in the routine that populates/uses
ids and maxCandidates) ensure maxCandidates is non-negative and clamp it to the
allowed range (e.g., if maxCandidates <= 0 set it to 0, and if maxCandidates >
len(ids) set it to len(ids)) so you only ever slice with 0 <= maxCandidates <=
len(ids); update the conditional around ids and maxCandidates to perform this
guard and handle the zero-case (empty result) or return an error if a negative
maxCandidates is invalid for the caller.
---
Nitpick comments:
In `@internal/transfer/vdb_router_test.go`:
- Around line 159-179: Update the TestVDBRouter_MaxCandidatesTruncation test to
include cases where maxCandidates is non-positive (e.g., 0 and a negative value)
and assert SuggestTransfer on router (the newTestRouter instance) does not panic
and returns either a nil suggestion or an empty SimilarIssues slice;
specifically exercise router.SuggestTransfer(context.Background(),
issueInput("foo","bar"), "org/current", 0.5, 1, 0) and with a negative
maxCandidates to ensure the slicing boundary in SuggestTransfer is handled
safely and no panic occurs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/core/config/config.gointernal/integrations/gemini/llm.gointernal/integrations/gemini/prompts.gointernal/steps/transfer_check.gointernal/steps/transfer_check_test.gointernal/transfer/vdb_router.gointernal/transfer/vdb_router_test.go
Comment on lines
+116
to
+122
| type VDBRoutingConfig struct { | ||
| Enabled *bool `yaml:"enabled,omitempty"` | ||
| ConfidenceThreshold float64 `yaml:"confidence_threshold,omitempty"` // Default: 0.75 | ||
| MinSamplesPerRepo int `yaml:"min_samples_per_repo,omitempty"` // Default: 20 | ||
| MaxCandidates int `yaml:"max_candidates,omitempty"` // Default: 3 | ||
| ExplainDecision bool `yaml:"explain_decision,omitempty"` // Default: true | ||
| } |
There was a problem hiding this comment.
ExplainDecision currently can’t support both “default true” and explicit false overrides.
Using a plain bool collapses “unset” and false, and the merge logic only copies true. This prevents child configs from explicitly disabling explanations and conflicts with the documented default behavior.
💡 Suggested fix
type VDBRoutingConfig struct {
Enabled *bool `yaml:"enabled,omitempty"`
ConfidenceThreshold float64 `yaml:"confidence_threshold,omitempty"` // Default: 0.75
MinSamplesPerRepo int `yaml:"min_samples_per_repo,omitempty"` // Default: 20
MaxCandidates int `yaml:"max_candidates,omitempty"` // Default: 3
- ExplainDecision bool `yaml:"explain_decision,omitempty"` // Default: true
+ ExplainDecision *bool `yaml:"explain_decision,omitempty"` // Default: true
} // VDB routing defaults
+ if c.Transfer.VDBRouting.ExplainDecision == nil {
+ t := true
+ c.Transfer.VDBRouting.ExplainDecision = &t
+ }- if child.Transfer.VDBRouting.ExplainDecision {
+ if child.Transfer.VDBRouting.ExplainDecision != nil {
result.Transfer.VDBRouting.ExplainDecision = child.Transfer.VDBRouting.ExplainDecision
}Also applies to: 431-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config/config.go` around lines 116 - 122, VDBRoutingConfig's
ExplainDecision is a plain bool so "unset" and false are indistinguishable and
the merge logic (which currently only copies true) prevents children from
explicitly disabling explanations; change ExplainDecision to *bool in
VDBRoutingConfig (and mirror this pointer change for the other affected bools
mentioned), update the config merge/merge-from logic to treat nil as "use
default" (default true) and to copy explicit false values from child configs,
and ensure any code reading ExplainDecision dereferences the pointer with a
default true when nil.
Comment on lines
+104
to
+146
| func TestTransferCheck_VDBFallback(t *testing.T) { | ||
| cfg := &config.Config{ | ||
| Qdrant: config.QdrantConfig{Collection: "test"}, | ||
| Transfer: config.TransferConfig{ | ||
| Enabled: boolPtr(true), | ||
| Rules: []config.TransferRule{}, // no rules | ||
| VDBRouting: config.VDBRoutingConfig{ | ||
| Enabled: boolPtr(true), | ||
| ConfidenceThreshold: 0.5, | ||
| MinSamplesPerRepo: 1, | ||
| MaxCandidates: 3, | ||
| }, | ||
| Strategy: "hybrid", | ||
| }, | ||
| } | ||
|
|
||
| // All VDB results point to "myorg/other-repo" (excluding current myorg/myrepo) | ||
| vdbResults := []*qdrant.SearchResult{ | ||
| {ID: "1", Score: 0.99, Payload: map[string]interface{}{"org": "myorg", "repo": "other-repo"}}, | ||
| {ID: "2", Score: 0.95, Payload: map[string]interface{}{"org": "myorg", "repo": "other-repo"}}, | ||
| {ID: "3", Score: 0.90, Payload: map[string]interface{}{"org": "myorg", "repo": "other-repo"}}, | ||
| } | ||
|
|
||
| // Provide a mock embedder via embedding via VectorStore mock that returns results | ||
| // We use a nil embedder here — the transfer package mock embedder only lives in transfer_test. | ||
| // Instead, inject a real VDBRouter would need an embedder. We test via integration: | ||
| // The step calls transfer.NewVDBRouter(s.embedder, ...) — s.embedder is nil here so VDB is skipped. | ||
| // To properly test VDB fallback we need an embedder shim in the steps package. | ||
| // For now assert that the step gracefully skips (no panic) when embedder is nil. | ||
| step := &TransferCheck{ | ||
| vectorStore: &tcMockStore{results: vdbResults}, | ||
| embedder: nil, // nil embedder → VDB skipped gracefully | ||
| } | ||
|
|
||
| pipeCtx := makeCtx(cfg, "some unrelated title") | ||
| if err := step.Run(pipeCtx); err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| // embedder is nil → VDB skipped, no target set | ||
| if pipeCtx.TransferTarget != "" { | ||
| t.Errorf("expected no transfer target when embedder nil, got %s", pipeCtx.TransferTarget) | ||
| } | ||
| } |
There was a problem hiding this comment.
TestTransferCheck_VDBFallback currently verifies skip behavior, not fallback routing.
This test passes with embedder=nil, so it doesn’t validate the VDB fallback objective. Either rename it to reflect the skip-path assertion, or add a real fallback test once TransferCheck accepts a mockable embedder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/steps/transfer_check_test.go` around lines 104 - 146, The test
TestTransferCheck_VDBFallback is actually asserting the skip-path when embedder
is nil, not VDB fallback; either rename the test to reflect that behavior (e.g.,
TestTransferCheck_SkipsVDBWhenEmbedderNil) or implement a real fallback test by
making TransferCheck accept a mockable embedder: inject a shim embedder into the
TransferCheck (instead of embedder: nil) and ensure transfer.NewVDBRouter is
created with that mock so tcMockStore vdbResults are used; then run
step.Run(makeCtx(...)) and assert pipeCtx.TransferTarget is set to the expected
repo ("myorg/other-repo"). Ensure you update references to TransferCheck,
NewVDBRouter, tcMockStore, makeCtx, Run and TransferTarget accordingly.
Comment on lines
+23
to
+26
| embedder *gemini.Embedder | ||
| vectorStore qdrant.VectorStore | ||
| llmClient *gemini.LLMClient | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use interfaces for embedder/llmClient dependencies to make VDB paths testable.
The concrete Gemini types make fallback/explain branches hard to unit test in this package. Small local interfaces would keep behavior unchanged while enabling proper mocks.
Also applies to: 29-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/steps/transfer_check.go` around lines 23 - 26, Replace the concrete
Gemini types with small local interfaces to allow mocking: define an Embedder
interface (e.g., with the methods used from gemini.Embedder) and an LLMClient
interface (with the methods used from *gemini.LLMClient*), update the struct
fields embedder and llmClient in transfer_check.go to use these interfaces, and
adapt any constructors or initializers to accept/assign the concrete gemini
implementations to those interfaces; ensure the concrete gemini.Embedder and
gemini.LLMClient still satisfy the new interfaces so runtime behavior is
unchanged and unit tests can inject mocks.
Comment on lines
+129
to
+130
| if len(ids) > maxCandidates { | ||
| ids = ids[:maxCandidates] |
There was a problem hiding this comment.
Guard maxCandidates bounds before slicing to avoid panic.
A negative maxCandidates can trigger slice bounds out of range at runtime.
🛠️ Suggested fix
+ if maxCandidates < 0 {
+ return nil, fmt.Errorf("vdb_router: maxCandidates must be >= 0")
+ }
ids := repoIDs[bestRepo]
- if len(ids) > maxCandidates {
+ if maxCandidates > 0 && len(ids) > maxCandidates {
ids = ids[:maxCandidates]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/transfer/vdb_router.go` around lines 129 - 130, The slice operation
ids = ids[:maxCandidates] can panic if maxCandidates is negative; before slicing
(in the routine that populates/uses ids and maxCandidates) ensure maxCandidates
is non-negative and clamp it to the allowed range (e.g., if maxCandidates <= 0
set it to 0, and if maxCandidates > len(ids) set it to len(ids)) so you only
ever slice with 0 <= maxCandidates <= len(ids); update the conditional around
ids and maxCandidates to perform this guard and handle the zero-case (empty
result) or return an error if a negative maxCandidates is invalid for the
caller.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #23
Adds VDB semantic search as a fallback to the
transfer_checkstep when no rule-based match is found. The two approaches are unified under a configurable strategy field (rules-only,vdb-only,hybrid).VDBRoutingConfigstruct (confidence_threshold,min_samples_per_repo,max_candidates,explain_decision) andStrategyfield onTransferConfigtransfer/vdb_router.go—VDBRouter.SuggestTransfer()embeds the issue, searches the VDB collection, analyses repo distribution of results and returns a candidate when confidence + sample-count thresholds are metgemini/llm.go+prompts.go—ExplainTransfer()method (temperature 0.3) generates a 2–3 sentence explanation for the transfer decisionsteps/transfer_check.go— hybrid flow: rules evaluated first; if no rule matches andstrategy=hybrid, VDB fallback is invoked; loop prevention andtransfer_blockedflag apply to both paths; setstransfer_method,transfer_confidence,transfer_reasoningmetadataCommits
feat(config)— VDBRoutingConfig + Strategy in TransferConfigfeat(transfer)— VDBRouter + unit testsfeat(gemini)— ExplainTransfer method + promptfeat(steps)— hybrid flow in transfer_check + integration testsTest plan
go build ./...compiles cleango test ./...all greenvdb_routing.enabled: true+strategy: hybrid, process an issue that matches no rules → verify VDB fallback log lines andtransfer_method=vdbin output🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes