actually deregister the inline completion provider when disabled

Author: markijbema

src/services/ghost/GhostServiceManager.ts

@@ -121,20 +121,21 @@ export class GhostServiceManager {
 		await this.saveSettings()
 	}
 
-	/**
-	 * Update the inline completion provider with current settings
-	 */
 	private async updateInlineCompletionProviderRegistration() {
-		// Always keep the provider registered so manual triggers (cmd-L) work
-		// The provider will check enableAutoTrigger internally to decide whether to auto-trigger
-		if (!this.inlineCompletionProviderDisposable) {
+		const shouldBeRegistered = this.settings?.enableAutoTrigger ?? false
+
+		if (shouldBeRegistered && !this.inlineCompletionProviderDisposable) {
+			// Register the provider
 			this.inlineCompletionProviderDisposable = vscode.languages.registerInlineCompletionItemProvider(
 				"*",
 				this.inlineCompletionProvider,
 			)
 			this.context.subscriptions.push(this.inlineCompletionProviderDisposable)
+		} else if (!shouldBeRegistered && this.inlineCompletionProviderDisposable) {
+			// Deregister the provider
+			this.inlineCompletionProviderDisposable.dispose()
+			this.inlineCompletionProviderDisposable = null
 		}
-		// No need to update settings - provider reads them dynamically via getSettings callback
 	}
 
 	public async disable() {

src/services/ghost/__tests__/GhostServiceManager.spec.ts

@@ -290,4 +290,113 @@ console.log('test');]]></replace></change>`
 			expect(mockProvider.provideInlineCompletionItems).not.toHaveBeenCalled()
 		})
 	})
+
+	describe("updateInlineCompletionProviderRegistration", () => {
+		it("should register provider when enableAutoTrigger is true", async () => {
+			const mockDisposable = { dispose: vi.fn() }
+			const mockRegister = vi.fn().mockReturnValue(mockDisposable)
+
+			const mockManager = {
+				settings: { enableAutoTrigger: true } as any,
+				inlineCompletionProviderDisposable: null as any,
+				inlineCompletionProvider: {} as any,
+				context: { subscriptions: [] as any[] },
+				async updateInlineCompletionProviderRegistration() {
+					const shouldBeRegistered = this.settings?.enableAutoTrigger ?? false
+
+					if (shouldBeRegistered && !this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable = mockRegister("*", this.inlineCompletionProvider)
+						this.context.subscriptions.push(this.inlineCompletionProviderDisposable)
+					} else if (!shouldBeRegistered && this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable.dispose()
+						this.inlineCompletionProviderDisposable = null
+					}
+				},
+			}
+
+			await mockManager.updateInlineCompletionProviderRegistration()
+
+			expect(mockRegister).toHaveBeenCalledWith("*", mockManager.inlineCompletionProvider)
+			expect(mockManager.inlineCompletionProviderDisposable).toBe(mockDisposable)
+			expect(mockManager.context.subscriptions).toContain(mockDisposable)
+		})
+
+		it("should deregister provider when enableAutoTrigger is false", async () => {
+			const mockDisposable = { dispose: vi.fn() }
+
+			const mockManager = {
+				settings: { enableAutoTrigger: false } as any,
+				inlineCompletionProviderDisposable: mockDisposable as any,
+				inlineCompletionProvider: {} as any,
+				context: { subscriptions: [mockDisposable] as any[] },
+				async updateInlineCompletionProviderRegistration() {
+					const shouldBeRegistered = this.settings?.enableAutoTrigger ?? false
+
+					if (shouldBeRegistered && !this.inlineCompletionProviderDisposable) {
+						// Register logic (not executed in this test)
+					} else if (!shouldBeRegistered && this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable.dispose()
+						this.inlineCompletionProviderDisposable = null
+					}
+				},
+			}
+
+			await mockManager.updateInlineCompletionProviderRegistration()
+
+			expect(mockDisposable.dispose).toHaveBeenCalled()
+			expect(mockManager.inlineCompletionProviderDisposable).toBeNull()
+		})
+
+		it("should not register provider twice when already registered", async () => {
+			const mockDisposable = { dispose: vi.fn() }
+			const mockRegister = vi.fn().mockReturnValue(mockDisposable)
+
+			const mockManager = {
+				settings: { enableAutoTrigger: true } as any,
+				inlineCompletionProviderDisposable: mockDisposable as any,
+				inlineCompletionProvider: {} as any,
+				context: { subscriptions: [mockDisposable] as any[] },
+				async updateInlineCompletionProviderRegistration() {
+					const shouldBeRegistered = this.settings?.enableAutoTrigger ?? false
+
+					if (shouldBeRegistered && !this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable = mockRegister("*", this.inlineCompletionProvider)
+						this.context.subscriptions.push(this.inlineCompletionProviderDisposable)
+					} else if (!shouldBeRegistered && this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable.dispose()
+						this.inlineCompletionProviderDisposable = null
+					}
+				},
+			}
+
+			await mockManager.updateInlineCompletionProviderRegistration()
+
+			expect(mockRegister).not.toHaveBeenCalled()
+			expect(mockManager.inlineCompletionProviderDisposable).toBe(mockDisposable)
+		})
+
+		it("should not deregister when already deregistered", async () => {
+			const mockManager = {
+				settings: { enableAutoTrigger: false } as any,
+				inlineCompletionProviderDisposable: null as any,
+				inlineCompletionProvider: {} as any,
+				context: { subscriptions: [] as any[] },
+				async updateInlineCompletionProviderRegistration() {
+					const shouldBeRegistered = this.settings?.enableAutoTrigger ?? false
+
+					if (shouldBeRegistered && !this.inlineCompletionProviderDisposable) {
+						// Register logic (not executed in this test)
+					} else if (!shouldBeRegistered && this.inlineCompletionProviderDisposable) {
+						this.inlineCompletionProviderDisposable.dispose()
+						this.inlineCompletionProviderDisposable = null
+					}
+				},
+			}
+
+			// Should not throw or cause issues
+			await mockManager.updateInlineCompletionProviderRegistration()
+
+			expect(mockManager.inlineCompletionProviderDisposable).toBeNull()
+		})
+	})
 })

src/services/ghost/classic-auto-complete/__tests__/GhostInlineCompletionProvider.test.ts

@@ -922,7 +922,7 @@ describe("GhostInlineCompletionProvider", () => {
 			expect(mockModel.generateResponse).not.toHaveBeenCalled()
 		})
 
-		it("should allow manual trigger even when auto-trigger is disabled", async () => {
+		it("should block manual trigger when auto-trigger is disabled (defense in depth)", async () => {
 			// Set auto-trigger to false
 			mockSettings = { enableAutoTrigger: false }
 
@@ -932,10 +932,17 @@ describe("GhostInlineCompletionProvider", () => {
 				selectedCompletionInfo: undefined,
 			} as vscode.InlineCompletionContext
 
-			await provider.provideInlineCompletionItems(mockDocument, mockPosition, manualContext, mockToken)
+			const result = await provider.provideInlineCompletionItems(
+				mockDocument,
+				mockPosition,
+				manualContext,
+				mockToken,
+			)
 
-			// Model should be called even though auto-trigger is disabled
-			expect(mockModel.generateResponse).toHaveBeenCalled()
+			// Should return empty array as defense in depth, even for manual triggers
+			// The provider should be deregistered at the manager level when disabled
+			expect(result).toEqual([])
+			expect(mockModel.generateResponse).not.toHaveBeenCalled()
 		})
 
 		it("should read settings dynamically on each call", async () => {