Skip to content

Commit 30dc598

Browse files
committed
fix: address review comments for PR #5041
- Add token limit validation in OpenAI and Ollama embedders to prevent exceeding MAX_ITEM_TOKENS with query prefixes - Add comprehensive test coverage for currentSearchMinScore getter priority system - Ensure fallback to unprefixed text when token limit would be exceeded - Add console warnings when falling back to unprefixed embeddings
1 parent f32a105 commit 30dc598

File tree

3 files changed

+169
-2
lines changed

3 files changed

+169
-2
lines changed

src/services/code-index/__tests__/config-manager.spec.ts

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,153 @@ describe("CodeIndexConfigManager", () => {
709709
const result = await configManager.loadConfiguration()
710710
expect(result.requiresRestart).toBe(false)
711711
})
712+
713+
describe("currentSearchMinScore priority system", () => {
714+
it("should return user-configured score when set", async () => {
715+
mockContextProxy.getGlobalState.mockReturnValue({
716+
codebaseIndexEnabled: true,
717+
codebaseIndexQdrantUrl: "http://qdrant.local",
718+
codebaseIndexEmbedderProvider: "openai",
719+
codebaseIndexEmbedderModelId: "text-embedding-3-small",
720+
codebaseIndexSearchMinScore: 0.8, // User setting
721+
})
722+
mockContextProxy.getSecret.mockImplementation((key: string) => {
723+
if (key === "codeIndexOpenAiKey") return "test-key"
724+
return undefined
725+
})
726+
727+
await configManager.loadConfiguration()
728+
expect(configManager.currentSearchMinScore).toBe(0.8)
729+
})
730+
731+
it("should fall back to model-specific threshold when user setting is undefined", async () => {
732+
mockContextProxy.getGlobalState.mockReturnValue({
733+
codebaseIndexEnabled: true,
734+
codebaseIndexQdrantUrl: "http://qdrant.local",
735+
codebaseIndexEmbedderProvider: "ollama",
736+
codebaseIndexEmbedderModelId: "nomic-embed-code",
737+
// No codebaseIndexSearchMinScore - user hasn't configured it
738+
})
739+
740+
await configManager.loadConfiguration()
741+
// nomic-embed-code has a specific threshold of 0.15
742+
expect(configManager.currentSearchMinScore).toBe(0.15)
743+
})
744+
745+
it("should fall back to default SEARCH_MIN_SCORE when neither user setting nor model threshold exists", async () => {
746+
mockContextProxy.getGlobalState.mockReturnValue({
747+
codebaseIndexEnabled: true,
748+
codebaseIndexQdrantUrl: "http://qdrant.local",
749+
codebaseIndexEmbedderProvider: "openai",
750+
codebaseIndexEmbedderModelId: "unknown-model", // Model not in profiles
751+
// No codebaseIndexSearchMinScore
752+
})
753+
mockContextProxy.getSecret.mockImplementation((key: string) => {
754+
if (key === "codeIndexOpenAiKey") return "test-key"
755+
return undefined
756+
})
757+
758+
await configManager.loadConfiguration()
759+
// Should fall back to default SEARCH_MIN_SCORE (0.4)
760+
expect(configManager.currentSearchMinScore).toBe(0.4)
761+
})
762+
763+
it("should respect user setting of 0 (edge case)", async () => {
764+
mockContextProxy.getGlobalState.mockReturnValue({
765+
codebaseIndexEnabled: true,
766+
codebaseIndexQdrantUrl: "http://qdrant.local",
767+
codebaseIndexEmbedderProvider: "ollama",
768+
codebaseIndexEmbedderModelId: "nomic-embed-code",
769+
codebaseIndexSearchMinScore: 0, // User explicitly sets 0
770+
})
771+
772+
await configManager.loadConfiguration()
773+
// Should return 0, not fall back to model threshold (0.15)
774+
expect(configManager.currentSearchMinScore).toBe(0)
775+
})
776+
777+
it("should use model-specific threshold with openai-compatible provider", async () => {
778+
mockContextProxy.getGlobalState.mockImplementation((key: string) => {
779+
if (key === "codebaseIndexConfig") {
780+
return {
781+
codebaseIndexEnabled: true,
782+
codebaseIndexQdrantUrl: "http://qdrant.local",
783+
codebaseIndexEmbedderProvider: "openai-compatible",
784+
codebaseIndexEmbedderModelId: "nomic-embed-code",
785+
// No codebaseIndexSearchMinScore
786+
}
787+
}
788+
if (key === "codebaseIndexOpenAiCompatibleBaseUrl") return "https://api.example.com/v1"
789+
return undefined
790+
})
791+
mockContextProxy.getSecret.mockImplementation((key: string) => {
792+
if (key === "codebaseIndexOpenAiCompatibleApiKey") return "test-api-key"
793+
return undefined
794+
})
795+
796+
await configManager.loadConfiguration()
797+
// openai-compatible provider also has nomic-embed-code with 0.15 threshold
798+
expect(configManager.currentSearchMinScore).toBe(0.15)
799+
})
800+
801+
it("should use default model ID when modelId is not specified", async () => {
802+
mockContextProxy.getGlobalState.mockReturnValue({
803+
codebaseIndexEnabled: true,
804+
codebaseIndexQdrantUrl: "http://qdrant.local",
805+
codebaseIndexEmbedderProvider: "openai",
806+
// No modelId specified
807+
// No codebaseIndexSearchMinScore
808+
})
809+
mockContextProxy.getSecret.mockImplementation((key: string) => {
810+
if (key === "codeIndexOpenAiKey") return "test-key"
811+
return undefined
812+
})
813+
814+
await configManager.loadConfiguration()
815+
// Should use default model (text-embedding-3-small) threshold (0.4)
816+
expect(configManager.currentSearchMinScore).toBe(0.4)
817+
})
818+
819+
it("should handle priority correctly: user > model > default", async () => {
820+
// Test 1: User setting takes precedence
821+
mockContextProxy.getGlobalState.mockReturnValue({
822+
codebaseIndexEnabled: true,
823+
codebaseIndexQdrantUrl: "http://qdrant.local",
824+
codebaseIndexEmbedderProvider: "ollama",
825+
codebaseIndexEmbedderModelId: "nomic-embed-code", // Has 0.15 threshold
826+
codebaseIndexSearchMinScore: 0.9, // User overrides
827+
})
828+
829+
await configManager.loadConfiguration()
830+
expect(configManager.currentSearchMinScore).toBe(0.9) // User setting wins
831+
832+
// Test 2: Model threshold when no user setting
833+
mockContextProxy.getGlobalState.mockReturnValue({
834+
codebaseIndexEnabled: true,
835+
codebaseIndexQdrantUrl: "http://qdrant.local",
836+
codebaseIndexEmbedderProvider: "ollama",
837+
codebaseIndexEmbedderModelId: "nomic-embed-code",
838+
// No user setting
839+
})
840+
841+
const newManager = new CodeIndexConfigManager(mockContextProxy)
842+
await newManager.loadConfiguration()
843+
expect(newManager.currentSearchMinScore).toBe(0.15) // Model threshold
844+
845+
// Test 3: Default when neither exists
846+
mockContextProxy.getGlobalState.mockReturnValue({
847+
codebaseIndexEnabled: true,
848+
codebaseIndexQdrantUrl: "http://qdrant.local",
849+
codebaseIndexEmbedderProvider: "openai",
850+
codebaseIndexEmbedderModelId: "custom-unknown-model",
851+
// No user setting, unknown model
852+
})
853+
854+
const anotherManager = new CodeIndexConfigManager(mockContextProxy)
855+
await anotherManager.loadConfiguration()
856+
expect(anotherManager.currentSearchMinScore).toBe(0.4) // Default
857+
})
858+
})
712859
})
713860

714861
describe("empty/missing API key handling", () => {

src/services/code-index/embedders/ollama.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ export class CodeIndexOllamaEmbedder implements IEmbedder {
2828

2929
// Apply model-specific query prefix if required
3030
const queryPrefix = getModelQueryPrefix("ollama", modelToUse)
31-
const processedTexts = queryPrefix ? texts.map((text) => `${queryPrefix}${text}`) : texts
31+
const processedTexts = queryPrefix
32+
? texts.map((text) => (text.startsWith(queryPrefix) ? text : `${queryPrefix}${text}`))
33+
: texts
3234

3335
try {
3436
// Note: Standard Ollama API uses 'prompt' for single text, not 'input' for array.

src/services/code-index/embedders/openai.ts

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,25 @@ export class OpenAiEmbedder extends OpenAiNativeHandler implements IEmbedder {
4040

4141
// Apply model-specific query prefix if required
4242
const queryPrefix = getModelQueryPrefix("openai", modelToUse)
43-
const processedTexts = queryPrefix ? texts.map((text) => `${queryPrefix}${text}`) : texts
43+
const processedTexts = queryPrefix
44+
? texts.map((text, index) => {
45+
const prefixedText = `${queryPrefix}${text}`
46+
const estimatedTokens = Math.ceil(prefixedText.length / 4)
47+
if (estimatedTokens > MAX_ITEM_TOKENS) {
48+
console.warn(
49+
t("embeddings:textWithPrefixExceedsTokenLimit", {
50+
index,
51+
estimatedTokens,
52+
maxTokens: MAX_ITEM_TOKENS,
53+
prefixLength: queryPrefix.length,
54+
}),
55+
)
56+
// Return original text without prefix to avoid exceeding limit
57+
return text
58+
}
59+
return prefixedText
60+
})
61+
: texts
4462

4563
const allEmbeddings: number[][] = []
4664
const usage = { promptTokens: 0, totalTokens: 0 }

0 commit comments

Comments
 (0)