From 69a705aa4268711f6b74093287bf92598c68d0e0 Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Wed, 23 Sep 2026 23:29:16 +0300 Subject: [PATCH 1/2] fix(code-index): distinguish unloaded configuration from disabled indexing (#1722) --- src/core/tools/CodebaseSearchTool.ts | 9 +++ .../__tests__/CodebaseSearchTool.spec.ts | 56 ++++++++++++++--- .../CodebaseSearchTool.workspace.spec.ts | 22 ++++++- .../__tests__/config-manager.spec.ts | 61 ++++++++++++++++++ .../code-index/__tests__/manager.spec.ts | 62 +++++++++++++++++++ src/services/code-index/config-manager.ts | 11 +++- src/services/code-index/manager.ts | 5 ++ 7 files changed, 213 insertions(+), 13 deletions(-) diff --git a/src/core/tools/CodebaseSearchTool.ts b/src/core/tools/CodebaseSearchTool.ts index 004c61f9f8..240782d64c 100644 --- a/src/core/tools/CodebaseSearchTool.ts +++ b/src/core/tools/CodebaseSearchTool.ts @@ -63,12 +63,21 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> { throw new Error("CodeIndexManager is not available.") } + // Settings defaults are not evidence that a fresh manager is explicitly disabled. + // Initialization belongs to the manager's owner, not the search tool. + if (!manager.isConfigurationLoaded) { + throw new Error("Code Indexing is not initialized for this workspace.") + } + if (!manager.isFeatureEnabled) { throw new Error("Code Indexing is disabled in the settings.") } if (!manager.isFeatureConfigured) { throw new Error("Code Indexing is not configured (Missing OpenAI Key or Qdrant URL).") } + if (!manager.isInitialized) { + throw new Error("Code Indexing is not initialized for this workspace.") + } const searchResults: VectorStoreSearchResult[] = await manager.searchIndex(query, directoryPrefix) diff --git a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts index 4b0a6ed01d..e7f6cd2e74 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts @@ -25,7 +25,15 @@ describe("CodebaseSearchTool", () => { let task: Task let context: vscode.ExtensionContext let callbacks: ToolCallbacks - let manager: Pick + let manager: Pick< + CodeIndexManager, + | "isFeatureEnabled" + | "isFeatureConfigured" + | "isConfigurationLoaded" + | "isInitialized" + | "initialize" + | "searchIndex" + > let deref: ReturnType> beforeEach(() => { @@ -61,8 +69,11 @@ describe("CodebaseSearchTool", () => { pushToolResult: vi.fn(), } manager = { + isConfigurationLoaded: true, isFeatureEnabled: true, isFeatureConfigured: true, + isInitialized: true, + initialize: vi.fn().mockResolvedValue({ requiresRestart: false }), searchIndex: vi.fn().mockResolvedValue([]), } vi.mocked(CodeIndexManagerRegistry.getOrCreate).mockReturnValue(manager as CodeIndexManager) @@ -70,7 +81,10 @@ describe("CodebaseSearchTool", () => { vi.mocked(vscode.workspace.asRelativePath).mockReturnValue("src/result.ts") }) - afterEach(() => vi.restoreAllMocks()) + afterEach(() => { + expect(manager.initialize).not.toHaveBeenCalled() + vi.restoreAllMocks() + }) function result(overrides: Partial = {}): VectorStoreSearchResult { return { @@ -174,7 +188,8 @@ describe("CodebaseSearchTool", () => { expect(task.say).not.toHaveBeenCalled() }) - it("reports disabled indexing without searching", async () => { + it.each([true, false])("reports disabled indexing without searching (initialized: %s)", async (initialized) => { + Object.defineProperty(manager, "isInitialized", { value: initialized }) Object.defineProperty(manager, "isFeatureEnabled", { value: false }) await tool.execute({ query }, task, callbacks) @@ -190,20 +205,38 @@ describe("CodebaseSearchTool", () => { expect(task.say).not.toHaveBeenCalled() }) - it("reports missing index configuration without searching", async () => { - Object.defineProperty(manager, "isFeatureConfigured", { value: false }) + it.each([true, false])( + "reports missing index configuration without searching (initialized: %s)", + async (initialized) => { + Object.defineProperty(manager, "isInitialized", { value: initialized }) + Object.defineProperty(manager, "isFeatureConfigured", { value: false }) + + await tool.execute({ query }, task, callbacks) + + expect(callbacks.handleError).toHaveBeenCalledExactlyOnceWith( + toolNamesSchema.enum.codebase_search, + new Error("Code Indexing is not configured (Missing OpenAI Key or Qdrant URL)."), + ) + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + expect(task.consecutiveMistakeCount).toBe(0) + expect(manager.searchIndex).not.toHaveBeenCalled() + expect(vscode.workspace.asRelativePath).not.toHaveBeenCalled() + expect(task.say).not.toHaveBeenCalled() + }, + ) + + it("reports configured but unready services without initializing or searching", async () => { + Object.defineProperty(manager, "isInitialized", { value: false }) await tool.execute({ query }, task, callbacks) + expect(manager.initialize).not.toHaveBeenCalled() expect(callbacks.handleError).toHaveBeenCalledExactlyOnceWith( toolNamesSchema.enum.codebase_search, - new Error("Code Indexing is not configured (Missing OpenAI Key or Qdrant URL)."), + new Error("Code Indexing is not initialized for this workspace."), ) - expect(callbacks.pushToolResult).not.toHaveBeenCalled() - expect(task.consecutiveMistakeCount).toBe(0) expect(manager.searchIndex).not.toHaveBeenCalled() - expect(vscode.workspace.asRelativePath).not.toHaveBeenCalled() - expect(task.say).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() }) it.each([undefined, "src", ""])( @@ -214,6 +247,7 @@ describe("CodebaseSearchTool", () => { return [] }) await tool.execute({ query, path }, task, callbacks) + expect(CodeIndexManagerRegistry.getOrCreate).toHaveBeenCalledExactlyOnceWith(context, "/task") expect(manager.searchIndex).toHaveBeenCalledExactlyOnceWith(query, path) expect(callbacks.pushToolResult).toHaveBeenCalledExactlyOnceWith( `No relevant code snippets found for the query: "${query}"`, @@ -253,6 +287,8 @@ describe("CodebaseSearchTool", () => { .mockReturnValueOnce("src/result.ts") .mockReturnValueOnce("lib/other.ts") await tool.execute({ query }, task, callbacks) + expect(CodeIndexManagerRegistry.getOrCreate).toHaveBeenCalledExactlyOnceWith(context, "/task") + expect(manager.searchIndex).toHaveBeenCalledExactlyOnceWith(query, undefined) expect(vscode.workspace.asRelativePath).toHaveBeenCalledTimes(2) expect(vscode.workspace.asRelativePath).toHaveBeenNthCalledWith(1, "/task/src/result.ts", false) expect(vscode.workspace.asRelativePath).toHaveBeenNthCalledWith(2, "/task/lib/other.ts", false) diff --git a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts index 5cfabb4c43..68e3f7ddc9 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts @@ -19,6 +19,9 @@ vi.mock("../../../services/code-index/manager", () => ({ CodeIndexManager: vi.fn().mockImplementation(function (workspacePath: string) { let initialized = false return { + get isConfigurationLoaded() { + return initialized + }, get isInitialized() { return initialized }, @@ -184,7 +187,7 @@ describe("CodebaseSearchTool workspace selection", () => { expect(callbacks.pushToolResult).toHaveBeenCalledTimes(3) }) - it("creates a manager for an external task path without initializing it", async () => { + it("reports a fresh external task manager as uninitialized without initializing or searching", async () => { Object.defineProperty(task, "cwd", { value: "/external-task" }) await new CodebaseSearchTool().execute({ query: "external match" }, task, callbacks) @@ -201,9 +204,24 @@ describe("CodebaseSearchTool workspace selection", () => { expect(getWorkspacePath).not.toHaveBeenCalled() expect(callbacks.handleError).toHaveBeenCalledExactlyOnceWith( toolNamesSchema.enum.codebase_search, - new Error("Code Indexing is disabled in the settings."), + new Error("Code Indexing is not initialized for this workspace."), ) expect(callbacks.pushToolResult).not.toHaveBeenCalled() expect(task.say).not.toHaveBeenCalled() }) + + it("searches an initialized external manager while background indexing is ongoing", async () => { + Object.defineProperty(task, "cwd", { value: "/external-task" }) + const manager = CodeIndexManagerRegistry.getOrCreate(provider.context, task.cwd)! + await manager.initialize(provider.contextProxy) + vi.mocked(manager.initialize).mockClear() + Object.defineProperty(manager, "state", { get: () => "Indexing" }) + + await new CodebaseSearchTool().execute({ query: "external match" }, task, callbacks) + + expect(manager.state).toBe("Indexing") + expect(manager.initialize).not.toHaveBeenCalled() + expect(manager.searchIndex).toHaveBeenCalledExactlyOnceWith("external match", undefined) + expect(callbacks.handleError).not.toHaveBeenCalled() + }) }) diff --git a/src/services/code-index/__tests__/config-manager.spec.ts b/src/services/code-index/__tests__/config-manager.spec.ts index 8b41dfe1d6..71b6e79e61 100644 --- a/src/services/code-index/__tests__/config-manager.spec.ts +++ b/src/services/code-index/__tests__/config-manager.spec.ts @@ -115,6 +115,67 @@ describe("CodeIndexConfigManager", () => { }) describe("loadConfiguration", () => { + it("does not mark the synchronous constructor snapshot as loaded", () => { + expect(configManager.isConfigurationLoaded).toBe(false) + }) + + it("marks disabled configuration loaded only after secrets refresh completes", async () => { + let finishRefresh!: () => void + mockContextProxy.refreshSecrets.mockReturnValue( + new Promise((resolve) => { + finishRefresh = resolve + }), + ) + + const loading = configManager.loadConfiguration() + expect(configManager.isConfigurationLoaded).toBe(false) + finishRefresh() + await loading + expect(configManager.isConfigurationLoaded).toBe(true) + expect(configManager.isFeatureEnabled).toBe(false) + }) + + it("marks enabled but unconfigured settings as loaded", async () => { + mockContextProxy.getGlobalState.mockReturnValue({ codebaseIndexEnabled: true }) + await configManager.loadConfiguration() + expect(configManager.isConfigurationLoaded).toBe(true) + expect(configManager.isFeatureEnabled).toBe(true) + expect(configManager.isFeatureConfigured).toBe(false) + }) + + it.each(["secrets", "configuration", "restart", "payload"] as const)( + "retains the last successful load state when %s fails", + async (stage) => { + const error = new Error(`${stage} failed`) + const failNextLoad = () => { + if (stage === "secrets") { + mockContextProxy.refreshSecrets.mockRejectedValueOnce(error) + } else if (stage === "configuration") { + mockContextProxy.getGlobalState.mockImplementationOnce(() => { + throw error + }) + } else if (stage === "restart") { + vi.spyOn(configManager, "doesConfigChangeRequireRestart").mockImplementationOnce(() => { + throw error + }) + } else { + vi.spyOn(configManager, "currentSearchMinScore", "get").mockImplementationOnce(() => { + throw error + }) + } + } + + failNextLoad() + await expect(configManager.loadConfiguration()).rejects.toThrow(error) + expect(configManager.isConfigurationLoaded).toBe(false) + await configManager.loadConfiguration() + expect(configManager.isConfigurationLoaded).toBe(true) + failNextLoad() + await expect(configManager.loadConfiguration()).rejects.toThrow(error) + expect(configManager.isConfigurationLoaded).toBe(true) + }, + ) + it("should load default configuration when no state exists", async () => { mockContextProxy.getGlobalState.mockReturnValue(undefined) mockContextProxy.getSecret.mockReturnValue(undefined) diff --git a/src/services/code-index/__tests__/manager.spec.ts b/src/services/code-index/__tests__/manager.spec.ts index 9faf06627e..952e205fdd 100644 --- a/src/services/code-index/__tests__/manager.spec.ts +++ b/src/services/code-index/__tests__/manager.spec.ts @@ -1,3 +1,5 @@ +import { ContextProxy } from "../../../core/config/ContextProxy" +import { makeExtensionContext } from "../../../test-utils/vscode" import { CodeIndexManager } from "../manager" import { CodeIndexManagerRegistry } from "../code-index-manager-registry" import { CodeIndexServiceFactory } from "../service-factory" @@ -168,6 +170,66 @@ describe("CodeIndexManager - handleSettingsChange regression", () => { CodeIndexManagerRegistry.disposeAll() }) + describe("configuration readiness", () => { + it("does not regain readiness when a detached configuration finishes loading", async () => { + let finishRefresh!: () => void + const contextProxy = new ContextProxy(makeExtensionContext()) + vi.spyOn(contextProxy, "getGlobalState").mockReturnValue({ codebaseIndexEnabled: false }) + vi.spyOn(contextProxy, "getSecret").mockReturnValue(undefined) + vi.spyOn(contextProxy, "refreshSecrets").mockReturnValue( + new Promise((resolve) => { + finishRefresh = resolve + }), + ) + + const initialization = manager.initialize(contextProxy) + expect(manager.isConfigurationLoaded).toBe(false) + await manager.recoverFromError() + finishRefresh() + await initialization + expect(manager.isConfigurationLoaded).toBe(false) + }) + + it("keeps failed configuration unloaded until a settings refresh succeeds", async () => { + const error = new Error("secrets refresh failed") + const contextProxy = new ContextProxy(makeExtensionContext()) + vi.spyOn(contextProxy, "getGlobalState").mockReturnValue({ codebaseIndexEnabled: false }) + vi.spyOn(contextProxy, "getSecret").mockReturnValue(undefined) + vi.spyOn(contextProxy, "refreshSecrets").mockRejectedValueOnce(error).mockResolvedValue(undefined) + + await expect(manager.initialize(contextProxy)).rejects.toThrow(error) + expect(manager.isConfigurationLoaded).toBe(false) + await manager.handleSettingsChange() + expect(manager.isConfigurationLoaded).toBe(true) + expect(manager.isInitialized).toBe(false) + }) + + it("marks disabled configuration loaded only after secrets refresh completes", async () => { + let finishRefresh!: () => void + const refresh = new Promise((resolve) => { + finishRefresh = resolve + }) + const contextProxy = new ContextProxy(makeExtensionContext()) + vi.spyOn(contextProxy, "getGlobalState").mockReturnValue({ codebaseIndexEnabled: false }) + vi.spyOn(contextProxy, "getSecret").mockReturnValue(undefined) + vi.spyOn(contextProxy, "refreshSecrets").mockReturnValue(refresh) + + expect(manager.isConfigurationLoaded).toBe(false) + const initialization = manager.initialize(contextProxy) + expect(contextProxy.refreshSecrets).toHaveBeenCalledOnce() + expect(manager.isConfigurationLoaded).toBe(false) + finishRefresh() + await initialization + + expect(manager.isConfigurationLoaded).toBe(true) + expect(manager.isFeatureEnabled).toBe(false) + expect(manager.isInitialized).toBe(false) + + await manager.recoverFromError() + expect(manager.isConfigurationLoaded).toBe(false) + }) + }) + describe("handleSettingsChange", () => { it("should log background indexing failures", async () => { const indexingError = new Error("indexing startup failed") diff --git a/src/services/code-index/config-manager.ts b/src/services/code-index/config-manager.ts index ce65d0407b..5b2b2c63e2 100644 --- a/src/services/code-index/config-manager.ts +++ b/src/services/code-index/config-manager.ts @@ -11,6 +11,13 @@ import { providerIdentifiers } from "@roo-code/types/provider-identifiers" * Handles loading, validating, and providing access to configuration values. */ export class CodeIndexConfigManager { + private _isConfigurationLoaded = false + + /** Whether at least one full asynchronous configuration load has succeeded. */ + public get isConfigurationLoaded(): boolean { + return this._isConfigurationLoaded + } + private codebaseIndexEnabled: boolean = false private embedderProvider: EmbedderProvider = providerIdentifiers.openai private modelId?: string @@ -207,7 +214,7 @@ export class CodeIndexConfigManager { const requiresRestart = this.doesConfigChangeRequireRestart(previousConfigSnapshot) - return { + const result = { configSnapshot: previousConfigSnapshot, currentConfig: { isConfigured: this.isConfigured(), @@ -228,6 +235,8 @@ export class CodeIndexConfigManager { }, requiresRestart, } + this._isConfigurationLoaded = true + return result } /** diff --git a/src/services/code-index/manager.ts b/src/services/code-index/manager.ts index fd3e6b0553..ee181b17d6 100644 --- a/src/services/code-index/manager.ts +++ b/src/services/code-index/manager.ts @@ -103,6 +103,11 @@ export class CodeIndexManager { return this._configManager?.isFeatureConfigured ?? false } + /** Whether configuration loading has succeeded, independently of service or indexing readiness. */ + public get isConfigurationLoaded(): boolean { + return this._configManager?.isConfigurationLoaded ?? false + } + public get isInitialized(): boolean { try { this.assertInitialized() From eb7b1f304a749cf2dda90705e503a7db615f917c Mon Sep 17 00:00:00 2001 From: gubin-dev Date: Fri, 25 Sep 2026 20:05:01 +0300 Subject: [PATCH 2/2] fix(code-index): clarify search readiness errors --- src/core/tools/CodebaseSearchTool.ts | 2 +- .../__tests__/CodebaseSearchTool.spec.ts | 19 +++++++++++++++++++ .../CodebaseSearchTool.workspace.spec.ts | 2 +- .../pr-review-state-workflow.test.ts | 3 ++- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/core/tools/CodebaseSearchTool.ts b/src/core/tools/CodebaseSearchTool.ts index 240782d64c..6084dd6b74 100644 --- a/src/core/tools/CodebaseSearchTool.ts +++ b/src/core/tools/CodebaseSearchTool.ts @@ -66,7 +66,7 @@ export class CodebaseSearchTool extends BaseTool<"codebase_search"> { // Settings defaults are not evidence that a fresh manager is explicitly disabled. // Initialization belongs to the manager's owner, not the search tool. if (!manager.isConfigurationLoaded) { - throw new Error("Code Indexing is not initialized for this workspace.") + throw new Error("Code Indexing configuration has not been loaded for this workspace.") } if (!manager.isFeatureEnabled) { diff --git a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts index e7f6cd2e74..bc88d44ec1 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.spec.ts @@ -188,6 +188,25 @@ describe("CodebaseSearchTool", () => { expect(task.say).not.toHaveBeenCalled() }) + it("reports configuration that has never loaded before checking settings or services", async () => { + Object.defineProperties(manager, { + isConfigurationLoaded: { value: false }, + isFeatureEnabled: { value: false }, + isFeatureConfigured: { value: false }, + isInitialized: { value: false }, + }) + + await tool.execute({ query }, task, callbacks) + + expect(callbacks.handleError).toHaveBeenCalledExactlyOnceWith( + toolNamesSchema.enum.codebase_search, + new Error("Code Indexing configuration has not been loaded for this workspace."), + ) + expect(manager.initialize).not.toHaveBeenCalled() + expect(manager.searchIndex).not.toHaveBeenCalled() + expect(callbacks.pushToolResult).not.toHaveBeenCalled() + }) + it.each([true, false])("reports disabled indexing without searching (initialized: %s)", async (initialized) => { Object.defineProperty(manager, "isInitialized", { value: initialized }) Object.defineProperty(manager, "isFeatureEnabled", { value: false }) diff --git a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts index 68e3f7ddc9..03789f7d40 100644 --- a/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts +++ b/src/core/tools/__tests__/CodebaseSearchTool.workspace.spec.ts @@ -204,7 +204,7 @@ describe("CodebaseSearchTool workspace selection", () => { expect(getWorkspacePath).not.toHaveBeenCalled() expect(callbacks.handleError).toHaveBeenCalledExactlyOnceWith( toolNamesSchema.enum.codebase_search, - new Error("Code Indexing is not initialized for this workspace."), + new Error("Code Indexing configuration has not been loaded for this workspace."), ) expect(callbacks.pushToolResult).not.toHaveBeenCalled() expect(task.say).not.toHaveBeenCalled() diff --git a/src/services/__tests__/pr-review-state-workflow.test.ts b/src/services/__tests__/pr-review-state-workflow.test.ts index 9cb178f774..af33981d50 100644 --- a/src/services/__tests__/pr-review-state-workflow.test.ts +++ b/src/services/__tests__/pr-review-state-workflow.test.ts @@ -443,7 +443,8 @@ function latestGateStatus(result: Awaited>) { describe("PR review-state workflow", () => { it("uses supported CodeRabbit access and review controls", () => { - expect(codeRabbitConfig.chat.allow_non_org_members).toBe(false) + // External contributors can interact with chat; review overrides remain restricted below. + expect(codeRabbitConfig.chat.allow_non_org_members).toBe(true) expect(codeRabbitConfig.reviews.pre_merge_checks.override_requested_reviewers_only).toBe(true) expect(codeRabbitConfig.reviews.auto_review.auto_pause_after_reviewed_commits).toBe(0) expect(codeRabbitConfig.reviews.auto_review.labels).toEqual(["coderabbit-review-active"])