From f20a844f849ed36b40afd2ab99f4946e0f73debd Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:36:56 +0000 Subject: [PATCH 1/7] fix(plugin): bounded provider-state wait, disposal guards, macrotask score scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review findings on #62 (Devin, Copilot, Codex): - ensureProviderState() had no timeout: a hung host bootstrap wedged auto-capture (isCapturing stuck true) and profile learning forever. Now bounded at 10s with a warn-and-proceed fallback. - startWebServer().then assigned webServer with no disposal guard: a server finishing after dispose ran orphaned with live toasts. Now stopped immediately on late resolution. - Idle events arriving after dispose scheduled new capture work; the event handler and capture/learning pipelines now check disposed state. - Initial recalculateAllScores ran on a Promise.resolve() microtask, which executes before the host resumes from awaiting the plugin factory — the startup-blocking claim was wrong. Now a setTimeout(0) macrotask, cleared on dispose. --- docs/CHANGELOG.md | 4 + src/index.ts | 24 ++- src/services/ai/opencode-provider.ts | 32 +++- src/services/auto-capture.ts | 3 +- src/services/user-memory-learning.ts | 3 +- tests/auto-capture.test.ts | 2 + tests/chat-message-mode.test.ts | 2 + tests/disposal-lifecycle.test.ts | 213 +++++++++++++++++++++++++++ tests/memory-conflicts.test.ts | 2 + tests/plugin-error-handling.test.ts | 2 + tests/provider-state-bound.test.ts | 55 +++++++ tests/transcript-idle-wiring.test.ts | 5 +- tests/user-memory-learning.test.ts | 2 + 13 files changed, 341 insertions(+), 8 deletions(-) create mode 100644 tests/disposal-lifecycle.test.ts create mode 100644 tests/provider-state-bound.test.ts diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e687d0e..936aca3 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - None yet. +### Fixed + +- **Plugin lifecycle hardened per code-review findings** — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. + ## [2.23.2] - 2026-09-08 ### Changed diff --git a/src/index.ts b/src/index.ts index e34ecaa..58e0660 100644 --- a/src/index.ts +++ b/src/index.ts @@ -33,7 +33,11 @@ import type { UserProfileData } from "./services/user-profile/types.js"; import type { SearchResult } from "./services/sqlite/types.js"; import { getLanguageName } from "./services/language-detector.js"; import type { MemoryScope } from "./services/client.js"; -import { setProviderStateInit } from "./services/ai/opencode-provider.js"; +import { + isPluginDisposed, + markPluginDisposed, + setProviderStateInit, +} from "./services/ai/opencode-provider.js"; async function showToast( ctx: PluginInput, @@ -125,6 +129,8 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { const tags = getTags(directory); let webServer: WebServer | null = null; const sessionIdleTimers = new Map(); + // Reset for repeated factory invocations (tests, host reloads). + markPluginDisposed(false); const GLOBAL_PLUGIN_WARMUP_KEY = Symbol.for("opencode-mem0.plugin.warmedup"); @@ -217,6 +223,11 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { apiKey: CONFIG.webServerApiKey, }) .then(async (server) => { + // Disposed before listening completed — do not resurrect state. + if (isPluginDisposed()) { + server.stop(); + return; + } webServer = server; const url = webServer.getUrl(); @@ -256,16 +267,19 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { }); } + let initialScoringTimer: ReturnType | undefined; // Start background memory scoring recalculation if (isConfigured() && CONFIG.memoryScoring.enabled) { startScoringRecalculation(); - void Promise.resolve().then(() => { + // setTimeout (macrotask), not a microtask: the host's await of this factory + // resumes before the scan runs, so plugin loading is not blocked by it. + initialScoringTimer = setTimeout(() => { try { recalculateAllScores(true); } catch (error) { log("Initial scoring recalculation failed", { error: String(error) }); } - }); + }, 0); } // Start memory lifecycle job (STM/LTM decay, promotion, archiving) @@ -283,6 +297,7 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { const shutdownHandler = async () => { delete (globalThis as any)[Symbol.for("opencode-mem0.shutdown")]; + markPluginDisposed(true); try { for (const timer of sessionIdleTimers.values()) { clearTimeout(timer); @@ -290,6 +305,7 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { sessionIdleTimers.clear(); stopScoringRecalculation(); stopLifecycleJob(); + if (initialScoringTimer) clearTimeout(initialScoringTimer); clearInterval(sessionCleanupTimer); if (webServer) { webServer.stop(); @@ -646,6 +662,8 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { event: async (input: { event: { type: string; properties?: Record } }) => { const event = input.event; + // Entry checkpoint: once disposed, no new idle/compaction work starts. + if (isPluginDisposed()) return; if (event.type === "session.idle") { await handleSessionIdle(event, ctx, directory, sessionIdleTimers, webServer); } diff --git a/src/services/ai/opencode-provider.ts b/src/services/ai/opencode-provider.ts index 35238fc..9771acd 100644 --- a/src/services/ai/opencode-provider.ts +++ b/src/services/ai/opencode-provider.ts @@ -17,12 +17,42 @@ let _connectedProviders: string[] = []; let providerStateInit: Promise = Promise.resolve(); +// Bounded: a hung host bootstrap must not wedge auto-capture/profile +// processing forever — after this timeout we proceed without provider state. +// ponytail: fixed timeout; make configurable if slow hosts become real. +const PROVIDER_STATE_TIMEOUT_MS = 10_000; + +let disposed = false; + export function setProviderStateInit(promise: Promise): void { providerStateInit = promise; } +export function markPluginDisposed(value: boolean): void { + disposed = value; +} + +export function isPluginDisposed(): boolean { + return disposed; +} + export async function ensureProviderState(): Promise { - await providerStateInit; + let timer: ReturnType | undefined; + try { + await Promise.race([ + providerStateInit, + new Promise((_, reject) => { + timer = setTimeout( + () => reject(new Error("provider state init timeout")), + PROVIDER_STATE_TIMEOUT_MS + ); + }), + ]); + } catch { + log("opencode provider state not ready in time — proceeding without it"); + } finally { + if (timer) clearTimeout(timer); + } } export function setStatePath(path: string): void { diff --git a/src/services/auto-capture.ts b/src/services/auto-capture.ts index 72dbc4e..0a8febc 100644 --- a/src/services/auto-capture.ts +++ b/src/services/auto-capture.ts @@ -131,8 +131,9 @@ export async function performAutoCapture( isCapturing = true; let claimedPromptId: string | null = null; try { - const { ensureProviderState } = await import("./ai/opencode-provider.js"); + const { ensureProviderState, isPluginDisposed } = await import("./ai/opencode-provider.js"); await ensureProviderState(); + if (isPluginDisposed()) return; const prompt = userPromptManager.getLastUncapturedPrompt(sessionID); if (!prompt) return; if (!userPromptManager.claimPrompt(prompt.id)) return; diff --git a/src/services/user-memory-learning.ts b/src/services/user-memory-learning.ts index cb81dce..d4c9abc 100644 --- a/src/services/user-memory-learning.ts +++ b/src/services/user-memory-learning.ts @@ -101,8 +101,9 @@ export async function performUserProfileLearning( isLearningRunning = true; try { - const { ensureProviderState } = await import("./ai/opencode-provider.js"); + const { ensureProviderState, isPluginDisposed } = await import("./ai/opencode-provider.js"); await ensureProviderState(); + if (isPluginDisposed()) return; const threshold = CONFIG.userProfileAnalysisInterval; const maxBatches = CONFIG.userProfileMaxBatchesPerIdle; diff --git a/tests/auto-capture.test.ts b/tests/auto-capture.test.ts index 20e7445..54ef8e9 100644 --- a/tests/auto-capture.test.ts +++ b/tests/auto-capture.test.ts @@ -63,6 +63,8 @@ vi.mock("../src/services/user-prompt/user-prompt-manager.js", () => ({ userPromptManager: mockUserPromptManager, })); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, isProviderConnected: (...args: unknown[]) => mockIsProviderConnected(...args), getStatePath: (...args: unknown[]) => mockGetStatePath(...args), generateStructuredOutput: (...args: unknown[]) => mockGenerateStructuredOutput(...args), diff --git a/tests/chat-message-mode.test.ts b/tests/chat-message-mode.test.ts index 3c0e28c..fb0f609 100644 --- a/tests/chat-message-mode.test.ts +++ b/tests/chat-message-mode.test.ts @@ -100,6 +100,8 @@ vi.mock("../src/services/logger.js", () => ({ })); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, setStatePath: vi.fn(), setConnectedProviders: vi.fn(), setProviderStateInit: vi.fn(), diff --git a/tests/disposal-lifecycle.test.ts b/tests/disposal-lifecycle.test.ts new file mode 100644 index 0000000..93d7846 --- /dev/null +++ b/tests/disposal-lifecycle.test.ts @@ -0,0 +1,213 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +vi.mock("../src/services/logger.js", () => ({ + log: vi.fn(), + warn: vi.fn(), +})); + +vi.mock("../src/services/client.js", () => ({ + memoryClient: { + warmup: vi.fn().mockResolvedValue(undefined), + close: vi.fn(), + }, +})); + +let disposed = false; + +vi.mock("../src/services/ai/opencode-provider.js", () => ({ + setStatePath: vi.fn(), + setConnectedProviders: vi.fn(), + setProviderStateInit: vi.fn(), + ensureProviderState: () => Promise.resolve(), + markPluginDisposed: (value: boolean) => { + disposed = value; + }, + isPluginDisposed: () => disposed, +})); + +const mocks = vi.hoisted(() => ({ + performAutoCapture: vi.fn().mockResolvedValue(undefined), + performTranscriptCapture: vi.fn().mockResolvedValue(undefined), + cleanupOldTranscripts: vi.fn(), + performUserProfileLearning: vi.fn().mockResolvedValue(undefined), + startWebServerDeferred: [] as Array<(server: unknown) => void>, + lastServer: null as { + getUrl: ReturnType; + isRunning: ReturnType; + isServerOwner: ReturnType; + setOnTakeoverCallback: ReturnType; + stop: ReturnType; + } | null, +})); + +vi.mock("../src/services/auto-capture.js", () => ({ + performAutoCapture: mocks.performAutoCapture, +})); + +vi.mock("../src/services/user-memory-learning.js", () => ({ + performUserProfileLearning: mocks.performUserProfileLearning, +})); + +vi.mock("../src/services/transcript-capture.js", () => ({ + performTranscriptCapture: mocks.performTranscriptCapture, + cleanupOldTranscripts: mocks.cleanupOldTranscripts, +})); + +vi.mock("../src/services/user-prompt/user-prompt-manager.js", () => ({ + userPromptManager: { + savePrompt: vi.fn(), + buildPrompt: vi.fn().mockReturnValue("test"), + pruneCapturedOlderThan: vi.fn().mockReturnValue(0), + }, +})); + +vi.mock("../src/services/context.js", () => ({ + formatContextForPrompt: vi.fn().mockReturnValue(""), +})); + +vi.mock("../src/services/tags.js", () => ({ + getTags: vi.fn().mockReturnValue({ project: { tag: "tag_project_test" } }), +})); + +vi.mock("../src/services/privacy.js", () => ({ + stripPrivateContent: vi.fn((x: string) => x), + isFullyPrivate: vi.fn().mockReturnValue(false), +})); + +vi.mock("../src/services/ai/session/ai-session-manager.js", () => ({ + getAISessionManager: () => ({ cleanupExpiredSessions: () => 0 }), +})); + +vi.mock("../src/services/embedding.js", () => ({ + embeddingService: { + embeddingAvailable: true, + isWarmedUp: true, + }, +})); + +vi.mock("../src/services/memory-scoring-service.js", () => ({ + startScoringRecalculation: vi.fn(), + stopScoringRecalculation: vi.fn(), + recalculateAllScores: vi.fn(), +})); + +vi.mock("../src/services/memory-lifecycle.js", () => ({ + startLifecycleJob: vi.fn(), + stopLifecycleJob: vi.fn(), + runLifecycleMaintenance: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("../src/services/cleanup-service.js", () => ({ + cleanupService: { + shouldRunCleanup: () => false, + runCleanup: vi.fn(), + }, +})); + +vi.mock("../src/services/sqlite/connection-manager.js", () => ({ + connectionManager: { checkpointAll: vi.fn() }, +})); + +let configWebServerEnabled = false; + +vi.mock("../src/config.js", () => ({ + isConfigured: () => true, + initConfig: vi.fn(), + get CONFIG() { + return { + get webServerEnabled() { + return configWebServerEnabled; + }, + webServerPort: 4747, + webServerHost: "127.0.0.1", + webServerApiKey: undefined, + warmupTimeoutMs: 100, + memoryScoring: { enabled: false }, + memoryLifecycle: { enabled: false }, + transcriptStorage: { enabled: true, maxAgeDays: 30 }, + autoCaptureEnabled: true, + profileLearningEnabled: true, + promptRetentionDays: 30, + compaction: { enabled: false, memoryLimit: 10 }, + chatMessage: { enabled: false }, + showAutoCaptureToasts: false, + }; + }, +})); + +vi.mock("../src/services/web-server.js", () => ({ + WebServer: vi.fn(), + startWebServer: vi.fn( + () => + new Promise((resolve) => { + // Resolved by the test — models slow listener startup + mocks.startWebServerDeferred.push(resolve); + }) + ), +})); + +import { OpenCodeMemPlugin } from "../src/index.js"; + +function makeCtx() { + return { + directory: "/test", + client: { + session: { prompt: vi.fn().mockResolvedValue({ success: true }) }, + tui: { showToast: vi.fn().mockResolvedValue(undefined) }, + path: { get: vi.fn().mockResolvedValue({ data: { state: "/test/.opencode" } }) }, + provider: { list: vi.fn().mockResolvedValue({ data: { connected: [] } }) }, + }, + }; +} + +describe("plugin disposal lifecycle (review findings D1/D2)", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + disposed = false; + configWebServerEnabled = false; + mocks.startWebServerDeferred = []; + mocks.lastServer = null; + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("idle event arriving after dispose starts no new work", async () => { + const plugin = await OpenCodeMemPlugin(makeCtx() as never); + if (!plugin.event) throw new Error("event hook missing"); + await plugin.dispose(); + + await plugin.event({ + event: { type: "session.idle", properties: { sessionID: "sess-after" } }, + }); + + // Idle handler debounces 10s — advance past it + await vi.advanceTimersByTimeAsync(10001); + + expect(mocks.performAutoCapture).not.toHaveBeenCalled(); + expect(mocks.performTranscriptCapture).not.toHaveBeenCalled(); + }); + + it("web server resolution after dispose is stopped, not resurrected", async () => { + configWebServerEnabled = true; + const plugin = await OpenCodeMemPlugin(makeCtx() as never); + await plugin.dispose(); + + expect(mocks.startWebServerDeferred.length).toBe(1); + const server = { + getUrl: vi.fn(() => "http://localhost:4747"), + isRunning: vi.fn(() => true), + isServerOwner: vi.fn(() => true), + setOnTakeoverCallback: vi.fn(), + stop: vi.fn(), + }; + mocks.lastServer = server; + mocks.startWebServerDeferred[0](server); + await vi.advanceTimersByTimeAsync(50); + + expect(server.stop).toHaveBeenCalled(); + expect(server.getUrl).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/memory-conflicts.test.ts b/tests/memory-conflicts.test.ts index c9a601b..74c6239 100644 --- a/tests/memory-conflicts.test.ts +++ b/tests/memory-conflicts.test.ts @@ -54,6 +54,8 @@ vi.mock("../src/services/logger.js", () => ({ })); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, isProviderConnected: vi.fn().mockReturnValue(false), getStatePath: vi.fn().mockReturnValue("/tmp/state.json"), generateStructuredOutput: vi.fn(), diff --git a/tests/plugin-error-handling.test.ts b/tests/plugin-error-handling.test.ts index cf9c05f..dcffc95 100644 --- a/tests/plugin-error-handling.test.ts +++ b/tests/plugin-error-handling.test.ts @@ -36,6 +36,8 @@ vi.mock("../src/services/web-server.js", () => ({ })); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, setStatePath: vi.fn(), setConnectedProviders: vi.fn(), setProviderStateInit: vi.fn(), diff --git a/tests/provider-state-bound.test.ts b/tests/provider-state-bound.test.ts new file mode 100644 index 0000000..ceb2441 --- /dev/null +++ b/tests/provider-state-bound.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + ensureProviderState, + markPluginDisposed, + setProviderStateInit, +} from "../src/services/ai/opencode-provider.js"; + +const logCalls: Array<{ message: string }> = []; +vi.mock("../src/services/logger.js", () => ({ + log: (message: string) => { + logCalls.push({ message }); + }, +})); + +vi.mock("ai", () => ({ + generateText: vi.fn(() => Promise.resolve({ output: {} })), + Output: { object: ({ schema }: { schema: unknown }) => ({ schema }) }, +})); + +describe("ensureProviderState is bounded when host init hangs (review finding C1)", () => { + beforeEach(() => { + vi.useFakeTimers(); + logCalls.length = 0; + }); + afterEach(() => { + vi.useRealTimers(); + setProviderStateInit(Promise.resolve()); + markPluginDisposed(false); + }); + + it("settles after the timeout when provider init never resolves", async () => { + setProviderStateInit(new Promise(() => {})); + + let settled = false; + const p = ensureProviderState().then(() => { + settled = true; + }); + + // Before the timeout it is still pending… + await vi.advanceTimersByTimeAsync(1); + expect(settled).toBe(false); + + // …and after the 10s bound it completes instead of wedging forever. + await vi.advanceTimersByTimeAsync(10_000); + await p; + expect(settled).toBe(true); + expect(logCalls.some((c) => c.message.includes("proceeding without it"))).toBe(true); + }); + + it("completes immediately when provider init already settled", async () => { + setProviderStateInit(Promise.resolve()); + await expect(ensureProviderState()).resolves.toBeUndefined(); + expect(logCalls.some((c) => c.message.includes("proceeding without it"))).toBe(false); + }); +}); diff --git a/tests/transcript-idle-wiring.test.ts b/tests/transcript-idle-wiring.test.ts index bb7f93b..3609d1a 100644 --- a/tests/transcript-idle-wiring.test.ts +++ b/tests/transcript-idle-wiring.test.ts @@ -29,6 +29,8 @@ vi.mock("../src/services/web-server.js", () => ({ })); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, setStatePath: vi.fn(), setConnectedProviders: vi.fn(), setProviderStateInit: vi.fn(), @@ -279,8 +281,7 @@ describe("session.idle transcript capture wiring", () => { expect(clearTimeoutSpy).toHaveBeenCalled(); expect(clearIntervalSpy).toHaveBeenCalled(); - const { stopScoringRecalculation } = - await import("../src/services/memory-scoring-service.js"); + const { stopScoringRecalculation } = await import("../src/services/memory-scoring-service.js"); const { stopLifecycleJob } = await import("../src/services/memory-lifecycle.js"); const { memoryClient } = await import("../src/services/client.js"); expect(stopScoringRecalculation).toHaveBeenCalled(); diff --git a/tests/user-memory-learning.test.ts b/tests/user-memory-learning.test.ts index 0c9f16f..51ee2c8 100644 --- a/tests/user-memory-learning.test.ts +++ b/tests/user-memory-learning.test.ts @@ -59,6 +59,8 @@ vi.mock("../src/services/user-prompt/user-prompt-manager.js", () => ({ const mockGenerateStructuredOutput = vi.fn(); vi.mock("../src/services/ai/opencode-provider.js", () => ({ + markPluginDisposed: vi.fn(), + isPluginDisposed: () => false, isProviderConnected: () => true, getStatePath: () => "/tmp/test-state", generateStructuredOutput: (...args: unknown[]) => mockGenerateStructuredOutput(...args), From c78740b81c24804c1507f124382747fa7d92b1be Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:39:35 +0000 Subject: [PATCH 2/7] fix(embedding): bound the warmup wait, degrade search softly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 on #62: embed() awaited warmup() with no bound — the AbortController in embedWithTimeout only guarded the API-fetch path, so a slow model download stalled the first chat.message hook past any configured timeout (and an abort during warmup left the service permanently disabled via embeddingAvailable=false). - warmup wait is raced against warmupTimeoutMs and the caller's AbortSignal; on either it rejects AbortError-shaped, which embed()'s catch rethrows without disabling the service. - searchMemories treats a warmup-pending AbortError as a degraded text-only search instead of propagating the failure to the prompt. --- docs/CHANGELOG.md | 4 ++- src/services/client.ts | 6 +++- src/services/embedding.ts | 32 ++++++++++++++++- tests/warmup-bound.test.ts | 71 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 tests/warmup-bound.test.ts diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 936aca3..6d0eb07 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,7 +11,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Plugin lifecycle hardened per code-review findings** — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. +- **Plugin lifecycle hardened per code-review findings** +- **Embedding warmup wait is bounded** — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. + — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. ## [2.23.2] - 2026-09-08 diff --git a/src/services/client.ts b/src/services/client.ts index 6f704ac..472f676 100644 --- a/src/services/client.ts +++ b/src/services/client.ts @@ -139,7 +139,11 @@ export class LocalMemoryClient { try { queryVector = await embeddingService.embedWithTimeout(query); } catch (error) { - if (!embeddingService.embeddingAvailable) { + // Warmup-wait timeouts surface as AbortError while the service is + // still healthy — degrade to text-only search instead of failing the + // prompt (the model load keeps running for the next attempt). + const warmupPending = error instanceof Error && error.name === "AbortError"; + if (!embeddingService.embeddingAvailable || warmupPending) { log("Embedding unavailable — falling back to text-only search", { queryLength: query.length, queryHash: query.slice(0, 20), diff --git a/src/services/embedding.ts b/src/services/embedding.ts index df4b95a..615fb84 100644 --- a/src/services/embedding.ts +++ b/src/services/embedding.ts @@ -119,7 +119,7 @@ export class EmbeddingService { try { if (!this.isWarmedUp) { - await this.warmup(); + await this.waitWarmupWithinBudget(signal); } if (CONFIG.embeddingApiUrl && CONFIG.embeddingApiKey) { @@ -172,6 +172,36 @@ export class EmbeddingService { clearTimeout(timeoutId); } } + // Bounded wait on model initialization: a slow or hung load must not + // stall callers indefinitely. Rejects AbortError-shaped so embed()'s + // catch rethrows without permanently disabling the service — the init + // promise keeps running, and the next call re-races it. + private async waitWarmupWithinBudget(signal?: AbortSignal): Promise { + let timer: ReturnType | undefined; + let onAbort: (() => void) | undefined; + try { + await Promise.race([ + this.warmup(), + new Promise((_, reject) => { + const giveUp = () => { + const err = new Error("embedding warmup wait timed out"); + err.name = "AbortError"; + reject(err); + }; + onAbort = () => { + const err = new Error("embedding warmup wait aborted"); + err.name = "AbortError"; + reject(err); + }; + signal?.addEventListener("abort", onAbort, { once: true }); + timer = setTimeout(giveUp, CONFIG.warmupTimeoutMs ?? 30_000); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + if (onAbort && signal) signal.removeEventListener("abort", onAbort); + } + } clearCache(): void { this.cache.clear(); diff --git a/tests/warmup-bound.test.ts b/tests/warmup-bound.test.ts new file mode 100644 index 0000000..0ea8530 --- /dev/null +++ b/tests/warmup-bound.test.ts @@ -0,0 +1,71 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { EmbeddingService } from "../src/services/embedding.js"; + +const logCalls: Array<{ message: string }> = []; +vi.mock("../src/services/logger.js", () => ({ + log: (message: string) => { + logCalls.push({ message }); + }, +})); + +let warmupTimeoutMs = 100; + +vi.mock("../src/config.js", () => ({ + get CONFIG() { + return { + get warmupTimeoutMs() { + return warmupTimeoutMs; + }, + storagePath: "/test", + embeddingModel: "test-model", + embeddingApiUrl: undefined, + embeddingApiKey: undefined, + }; + }, +})); + +const hangingPipeline = new Promise(() => {}); +vi.mock("@huggingface/transformers", () => ({ + pipeline: () => hangingPipeline, + env: {}, +})); + +describe("embed() warmup wait is bounded (review finding Codex-P1 on #62)", () => { + let service: EmbeddingService; + + beforeEach(() => { + vi.useFakeTimers(); + logCalls.length = 0; + warmupTimeoutMs = 100; + // Fresh instance per test — the singleton caches model/pipeline state. + service = new EmbeddingService(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it("rejects with AbortError and stays enabled when warmup never finishes", async () => { + const warmupWait = service.embed("something").catch((e: unknown) => e); + + await vi.advanceTimersByTimeAsync(1); + const early = await Promise.race([warmupWait, "pending"]); + expect(early).toBe("pending"); + + await vi.advanceTimersByTimeAsync(warmupTimeoutMs); + const err = await warmupWait; + expect(err).toBeInstanceOf(Error); + expect((err as Error).name).toBe("AbortError"); + // Crucially not disabled — the model load is still running and the next + // call may succeed once it lands. + expect((service as unknown as { embeddingAvailable: boolean }).embeddingAvailable).toBe(true); + }); + + it("degrade-on-abort: caller's signal interrupts the warmup wait", async () => { + const controller = new AbortController(); + const embedPromise = service.embed("something", controller.signal).catch((e: unknown) => e); + controller.abort(); + const err = await embedPromise; + expect((err as Error).name).toBe("AbortError"); + expect((service as unknown as { embeddingAvailable: boolean }).embeddingAvailable).toBe(true); + }); +}); From 54f4bc30ee04151f67d79be1ebba3d9f17e24c80 Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:41:00 +0000 Subject: [PATCH 3/7] fix(sqlite): updateVector keeps tags_vector on content-only updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin finding on #63: updateVector always wrote tags_vector, so a content-only re-embed (migration, admin re-embed) passed undefined and null'd the tag embeddings — semantic tag search silently disappeared for every updated memory. COALESCE preserves the stored blob when the caller provides none. --- docs/CHANGELOG.md | 4 +- src/services/sqlite/vector-search.ts | 11 +-- tests/update-tags-preserve.test.ts | 108 +++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 6 deletions(-) create mode 100644 tests/update-tags-preserve.test.ts diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 6d0eb07..9da5129 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -12,7 +12,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Plugin lifecycle hardened per code-review findings** -- **Embedding warmup wait is bounded** — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. +- **Embedding warmup wait is bounded** +- **`updateVector` no longer wipes `tags_vector` when only the content embedding is refreshed** — semantic tag search survives content-only re-embeds (Devin finding on #63). + — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. ## [2.23.2] - 2026-09-08 diff --git a/src/services/sqlite/vector-search.ts b/src/services/sqlite/vector-search.ts index 348d1e8..8344abf 100644 --- a/src/services/sqlite/vector-search.ts +++ b/src/services/sqlite/vector-search.ts @@ -650,11 +650,12 @@ export class VectorSearch { db.run("BEGIN IMMEDIATE"); try { - this.getStmt(db, "UPDATE memories SET vector = ?, tags_vector = ? WHERE id = ?").run( - toBlob(vector), - toBlob(tagsVector), - memoryId - ); + // COALESCE keeps the existing tags_vector when this call only re-embeds + // content — passing undefined must not wipe semantic tag search. + this.getStmt( + db, + "UPDATE memories SET vector = ?, tags_vector = COALESCE(?, tags_vector) WHERE id = ?" + ).run(toBlob(vector), toBlob(tagsVector), memoryId); db.run("COMMIT"); } catch (error) { try { diff --git a/tests/update-tags-preserve.test.ts b/tests/update-tags-preserve.test.ts new file mode 100644 index 0000000..ede5444 --- /dev/null +++ b/tests/update-tags-preserve.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +vi.mock("../src/services/sqlite/shard-manager.js", () => ({ + shardManager: { + getAllShards: vi.fn(), + getWriteShard: vi.fn(), + deleteShard: vi.fn(), + incrementVectorCount: vi.fn(), + }, +})); + +import { connectionManager } from "../src/services/sqlite/connection-manager.js"; +import { vectorSearch } from "../src/services/sqlite/vector-search.js"; + +function blob(values: number[]): Uint8Array { + return new Uint8Array(new Float32Array(values).buffer); +} + +describe("updateVector preserves tags_vector when not provided (Devin finding on #63)", () => { + let dir: string; + let dbPath: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "tags-preserve-")); + dbPath = join(dir, "s.db"); + const db = connectionManager.getConnection(dbPath); + db.run(` + CREATE TABLE IF NOT EXISTS memories ( + id TEXT PRIMARY KEY, + content TEXT NOT NULL, + vector BLOB NOT NULL, + tags_vector BLOB, + container_tag TEXT NOT NULL, + tags TEXT, + type TEXT, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + metadata TEXT, + display_name TEXT, + user_name TEXT, + user_email TEXT, + project_path TEXT, + project_name TEXT, + git_repo_url TEXT, + is_pinned INTEGER DEFAULT 0, + is_deprecated INTEGER DEFAULT 0 + ) + `); + const now = Date.now(); + db.run( + `INSERT INTO memories (id, content, vector, tags_vector, container_tag, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + "m1", + "one", + blob([1, 2, 3, 4]), + blob([9, 9, 9, 9]), + "mem_user_h", + now, + now + ); + }); + + afterEach(() => { + connectionManager.closeAll(); + rmSync(dir, { recursive: true, force: true }); + }); + + it("content-only re-embed leaves tags_vector intact (direct)", async () => { + const db = connectionManager.getConnection(dbPath); + await vectorSearch.updateVector(db, "m1", new Float32Array([5, 6, 7, 8])); + + const row = db.prepare("SELECT tags_vector FROM memories WHERE id = 'm1'").get() as { + tags_vector: Uint8Array; + }; + expect(row.tags_vector).not.toBeNull(); + expect( + Array.from(new Float32Array(row.tags_vector.buffer, row.tags_vector.byteOffset, 4)) + ).toEqual([9, 9, 9, 9]); + + const vecRow = db.prepare("SELECT vector FROM memories WHERE id = 'm1'").get() as { + vector: Uint8Array; + }; + expect(Array.from(new Float32Array(vecRow.vector.buffer, vecRow.vector.byteOffset, 4))).toEqual( + [5, 6, 7, 8] + ); + }); + + it("explicit tagsVector still overwrites", async () => { + const db = connectionManager.getConnection(dbPath); + await vectorSearch.updateVector( + db, + "m1", + new Float32Array([1, 1, 1, 1]), + undefined, + new Float32Array([7, 7, 7, 7]) + ); + + const row = db.prepare("SELECT tags_vector FROM memories WHERE id = 'm1'").get() as { + tags_vector: Uint8Array; + }; + expect( + Array.from(new Float32Array(row.tags_vector.buffer, row.tags_vector.byteOffset, 4)) + ).toEqual([7, 7, 7, 7]); + }); +}); From 7b2835b5039841c38458bacc3f036cda37ee77d5 Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:42:20 +0000 Subject: [PATCH 4/7] perf(sqlite): scope memories_fts update trigger to content, tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot/Codex finding on #63: AFTER UPDATE ON memories (unscoped) fired on every UPDATE — every access_count bump on a search hit and every decay/score touch — re-tokenizing the full content via FTS delete+reinsert. Scoped to UPDATE OF content, tags; DROP+CREATE in ensureMemoriesFts upgrades databases created with the old trigger. --- docs/CHANGELOG.md | 4 ++- src/services/sqlite/schema.ts | 9 +++++-- tests/memories-fts.test.ts | 49 +++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9da5129..fc19f17 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -13,7 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Plugin lifecycle hardened per code-review findings** - **Embedding warmup wait is bounded** -- **`updateVector` no longer wipes `tags_vector` when only the content embedding is refreshed** — semantic tag search survives content-only re-embeds (Devin finding on #63). +- \*\*`updateVector` no longer wipes +- **The `memories_fts` update trigger is column-scoped** — routine `access_count`/score updates on every search result and decay cycle no longer re-tokenize content, cutting FTS write amplification (Copilot/Codex finding on #63). + `tags_vector` when only the content embedding is refreshed\*\* — semantic tag search survives content-only re-embeds (Devin finding on #63). — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. diff --git a/src/services/sqlite/schema.ts b/src/services/sqlite/schema.ts index a133943..39a57b7 100644 --- a/src/services/sqlite/schema.ts +++ b/src/services/sqlite/schema.ts @@ -71,9 +71,14 @@ export function ensureMemoriesFts(db: Database): void { END `); + // Scoped to the indexed columns: an unscoped AFTER UPDATE fired on every + // UPDATE — including access_count/score touches on every search and decay + // cycle — re-tokenizing content on each (Copilot/Codex finding on #63). + // DROP+CREATE replaces the unscoped trigger from older databases. + db.run("DROP TRIGGER IF EXISTS memories_fts_update"); db.run(` - CREATE TRIGGER IF NOT EXISTS memories_fts_update - AFTER UPDATE ON memories BEGIN + CREATE TRIGGER memories_fts_update + AFTER UPDATE OF content, tags ON memories BEGIN INSERT INTO memories_fts(memories_fts, rowid, id, content, tags) VALUES ('delete', old.rowid, old.id, old.content, old.tags); INSERT INTO memories_fts(rowid, id, content, tags) diff --git a/tests/memories-fts.test.ts b/tests/memories-fts.test.ts index 8861a18..4e31e31 100644 --- a/tests/memories-fts.test.ts +++ b/tests/memories-fts.test.ts @@ -154,4 +154,53 @@ describe("memories_fts", () => { .get("alpha") as { id: string } | undefined; expect(hit?.id).toBe("old-1"); }); + + it("update trigger is scoped to content/tags — metadata updates don't rewrite FTS", () => { + const dir = mkdtempSync(join(tmpdir(), "memories-fts-scope-")); + dirs.push(dir); + const dbPath = join(dir, "scope.db"); + + const raw = new Database(dbPath); + raw.run(MEMORIES_DDL); + raw.close(); + + const db = connectionManager.getConnection(dbPath); + + // The trigger itself must be column-scoped. + const trig = db + .prepare("SELECT sql FROM sqlite_master WHERE type='trigger' AND name='memories_fts_update'") + .get() as { sql: string }; + expect(trig.sql).toContain("AFTER UPDATE OF content, tags ON memories"); + + const now = Date.now(); + db.run( + `INSERT INTO memories (id, content, vector, container_tag, tags, created_at, updated_at, is_deprecated) + VALUES (?, ?, ?, ?, ?, ?, ?, 0)`, + "m1", + "unscoped keyword original", + new Uint8Array(16), + "mem_user_ftstest", + "alpha", + now, + now + ); + + // Metadata-only update (as every search/decay cycle does): + db.run("UPDATE memories SET access_count = access_count + 1 WHERE id = 'm1'"); + const stillThere = db + .prepare("SELECT id FROM memories_fts WHERE memories_fts MATCH ?") + .get("unscoped") as { id: string } | undefined; + expect(stillThere?.id).toBe("m1"); + + // Content update must sync the index. + db.run("UPDATE memories SET content = 'brandnewcontent cylindercat' WHERE id = 'm1'"); + const oldHit = db + .prepare("SELECT id FROM memories_fts WHERE memories_fts MATCH ?") + .get("unscoped") as { id: string } | undefined; + expect(oldHit).toBeUndefined(); + const newHit = db + .prepare("SELECT id FROM memories_fts WHERE memories_fts MATCH ?") + .get("cylindercat") as { id: string } | undefined; + expect(newHit?.id).toBe("m1"); + }); }); From cc41f56a936249dab672de803e0703e3ea63f2de Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:46:01 +0000 Subject: [PATCH 5/7] fix(sqlite): honest post-commit index failures, force-rebuild dirty shards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Devin finding on #63: after COMMIT, a failing backend insert rejected the whole write although the sqlite row was durable — callers reported failure and retried into duplicates. Worse, the rebuild-dirty flag could never repair an initialized index: rebuildFromShard returned early for initialized indexes, so a dirty shard stayed missing entries until process restart. - rebuildFromShard gains force: replaces an initialized index by rebuilding from sqlite (the durable truth). - maybeRebuild distinguishes bootstrap (first use) from repair (dirty) and passes force only for the latter — steady-state searches never pay a rebuild. - insertVector/updateVector/replaceVector swallow post-commit backend failures with a warn + dirty mark; pre-commit sqlite failures still throw (nothing persisted). - markShardDirty() public — the re-embed migration uses it (next commit). --- docs/CHANGELOG.md | 4 +- src/services/sqlite/vector-search.ts | 46 +++++- src/services/vector-backends/types.ts | 8 +- .../vector-backends/usearch-backend.ts | 9 +- tests/backend-repair.test.ts | 153 ++++++++++++++++++ 5 files changed, 210 insertions(+), 10 deletions(-) create mode 100644 tests/backend-repair.test.ts diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index fc19f17..aee4baa 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -14,7 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Plugin lifecycle hardened per code-review findings** - **Embedding warmup wait is bounded** - \*\*`updateVector` no longer wipes -- **The `memories_fts` update trigger is column-scoped** — routine `access_count`/score updates on every search result and decay cycle no longer re-tokenize content, cutting FTS write amplification (Copilot/Codex finding on #63). +- **The `memories_fts` update trigger is column-scoped** +- **Vector-index writes after a sqlite COMMIT no longer misreport failure** — a failing backend insert used to reject `insertVector`/`updateVector`/`replaceVector` even though the memory was durably persisted (callers retried into duplicates), and the rebuild-dirty flag never actually repaired an initialized USearch index because `rebuildFromShard` skipped initialized indexes. Dirty shards now force a full index rebuild from sqlite on the next search (Devin finding on #63). + — routine `access_count`/score updates on every search result and decay cycle no longer re-tokenize content, cutting FTS write amplification (Copilot/Codex finding on #63). `tags_vector` when only the content embedding is refreshed\*\* — semantic tag search survives content-only re-embeds (Devin finding on #63). — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. — provider-state waits are bounded (a hung host bootstrap can no longer wedge auto-capture or profile learning forever), idle events arriving after `dispose` start no new work, a web server that finishes starting after `dispose` is stopped instead of running orphaned, and the initial score recalculation runs on a macrotask so the host receives the plugin before the shard scan. diff --git a/src/services/sqlite/vector-search.ts b/src/services/sqlite/vector-search.ts index 8344abf..a6b56cd 100644 --- a/src/services/sqlite/vector-search.ts +++ b/src/services/sqlite/vector-search.ts @@ -1,6 +1,6 @@ import { StmtCache, type Database } from "./sqlite-bootstrap.js"; import { connectionManager } from "./connection-manager.js"; -import { log } from "../logger.js"; +import { log, warn } from "../logger.js"; import { CONFIG } from "../../config.js"; import type { MemoryMetadata, MemoryRecord, SearchResult, ShardInfo } from "./types.js"; import { createVectorBackend } from "../vector-backends/backend-factory.js"; @@ -136,12 +136,26 @@ export class VectorSearch { // about existing sqlite rows, so first use must rebuild once. Without // this, vector search silently returned [] after every restart until a // new insert happened to land in that shard/kind. - if (this.rebuildDirty.get(key) !== false) { - await backend.rebuildFromShard({ db, shard, kind }); + const state = this.rebuildDirty.get(key); + if (state !== false) { + // state === undefined: first use in this process (bootstrap rebuild). + // state === true: a backend mutation failed after COMMIT — force a + // full repair pass that replaces the initialized index. + await backend.rebuildFromShard({ db, shard, kind, force: state === true }); this.rebuildDirty.set(key, false); } } + /** + * Marks a shard's backend indexes as needing a force rebuild on next + * search. Used by re-embed migrations: sqlite is the durable truth, and + * any initialized in-memory index may hold stale (old-dimension) vectors. + */ + markShardDirty(shard: ShardInfo): void { + this.rebuildDirty.set(`${shard.id}:content`, true); + this.rebuildDirty.set(`${shard.id}:tags`, true); + } + async insertVector(db: Database, record: MemoryRecord, shard?: ShardInfo): Promise { const insertMemory = this.getStmt(db, MEMORIES_INSERT_SQL); const backend = shard ? await this.getBackend() : undefined; @@ -165,7 +179,17 @@ export class VectorSearch { if (record.tagsVector) { await backend.insert({ id: record.id, vector: record.tagsVector, shard, kind: "tags" }); } - } finally { + } catch (error) { + // The sqlite row is durable — the post-commit index mutation is + // not. A failing backend insert used to reject the whole call even + // though the memory was persisted, so callers retried into + // duplicates. Mark the shard dirty instead; the next search rebuilds + // it from sqlite (force pass replaces the initialized index). + warn("Vector backend insert failed after COMMIT — index marked dirty for rebuild", { + shardId: shard.id, + memoryId: record.id, + error: String(error), + }); this.rebuildDirty.set(`${shard.id}:content`, true); this.rebuildDirty.set(`${shard.id}:tags`, true); } @@ -675,9 +699,13 @@ export class VectorSearch { await backend.delete({ id: memoryId, shard, kind: "tags" }); } } catch (error) { + warn("Vector backend update failed after COMMIT — index marked dirty for rebuild", { + shardId: shard.id, + memoryId, + error: String(error), + }); this.rebuildDirty.set(`${shard.id}:content`, true); this.rebuildDirty.set(`${shard.id}:tags`, true); - throw error; } } } @@ -714,9 +742,15 @@ export class VectorSearch { await backend.insert({ id: record.id, vector: record.tagsVector, shard, kind: "tags" }); } } catch (error) { + // Same honesty as insertVector: the sqlite mutation is durable; + // mark dirty for a force rebuild instead of rejecting a committed write. + warn("Vector backend update failed after COMMIT — index marked dirty for rebuild", { + shardId: shard.id, + memoryId: record.id, + error: String(error), + }); this.rebuildDirty.set(`${shard.id}:content`, true); this.rebuildDirty.set(`${shard.id}:tags`, true); - throw error; } } } diff --git a/src/services/vector-backends/types.ts b/src/services/vector-backends/types.ts index 17bdb2d..86d832c 100644 --- a/src/services/vector-backends/types.ts +++ b/src/services/vector-backends/types.ts @@ -35,7 +35,13 @@ export interface VectorBackend { }): void | Promise; delete(args: { id: string; shard: ShardInfo; kind: VectorKind }): void | Promise; search(args: VectorBackendSearchParams): BackendSearchResult[] | Promise; - rebuildFromShard(args: { db: unknown; shard: ShardInfo; kind: VectorKind }): void | Promise; + rebuildFromShard(args: { + db: unknown; + shard: ShardInfo; + kind: VectorKind; + /** Repair pass: replace an already-initialized index instead of skipping it. */ + force?: boolean; + }): void | Promise; deleteShardIndexes(args: { shard: ShardInfo }): void | Promise; } diff --git a/src/services/vector-backends/usearch-backend.ts b/src/services/vector-backends/usearch-backend.ts index 0e841da..c6f9b9f 100644 --- a/src/services/vector-backends/usearch-backend.ts +++ b/src/services/vector-backends/usearch-backend.ts @@ -91,10 +91,15 @@ export class USearchBackend implements VectorBackend { } } - async rebuildFromShard(args: { db: unknown; shard: ShardInfo; kind: VectorKind }): Promise { + async rebuildFromShard(args: { + db: unknown; + shard: ShardInfo; + kind: VectorKind; + force?: boolean; + }): Promise { const indexKey = getIndexKey(args.shard, args.kind); const existing = this.indexes.get(indexKey); - if (existing?.initialized) { + if (existing?.initialized && !args.force) { return; } diff --git a/tests/backend-repair.test.ts b/tests/backend-repair.test.ts new file mode 100644 index 0000000..e03190e --- /dev/null +++ b/tests/backend-repair.test.ts @@ -0,0 +1,153 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { VectorBackend } from "../src/services/vector-backends/types.js"; +import type { ShardInfo } from "../src/services/sqlite/types.js"; +import { connectionManager } from "../src/services/sqlite/connection-manager.js"; +import { VectorSearch } from "../src/services/sqlite/vector-search.js"; +import type { MemoryRecord } from "../src/services/sqlite/types.js"; + +vi.mock("../src/services/sqlite/shard-manager.js", () => ({ + shardManager: { getAllShards: vi.fn(), getWriteShard: vi.fn(), deleteShard: vi.fn() }, +})); + +vi.mock("../src/services/logger.js", () => ({ + log: vi.fn(), + warn: vi.fn(), +})); + +const shard: ShardInfo = { + id: 7, + scope: "user", + scopeHash: "h", + shardIndex: 0, + dbPath: "/tmp/repair-shard.db", + vectorCount: 0, + isActive: true, + createdAt: Date.now(), +}; + +function record(id: string, first = 1): MemoryRecord { + return { + id, + content: `content ${id}`, + vector: new Float32Array([first, 2, 3, 4]), + tagsVector: new Float32Array([9, 9, 9, 9]), + containerTag: "mem_user_h", + tags: "alpha", + type: "semantic", + createdAt: Date.now(), + updatedAt: Date.now(), + metadata: "{}", + displayName: null, + userName: null, + userEmail: null, + projectPath: null, + projectName: null, + gitRepoUrl: null, + isPinned: 0, + isDeprecated: 0, + recency: 0.5, + frequency: 0.5, + importance: 0.5, + utility: 0.5, + novelty: 0.5, + confidence: 0.5, + interferencePenalty: 0, + strength: 0.5, + accessCount: 0, + lastAccessed: null, + storeType: "stm", + decayRate: null, + } as unknown as MemoryRecord; +} + +function makeBackend(): VectorBackend & { + insertCalls: number; + rebuildCalls: Array<{ kind: string; force: boolean | undefined }>; +} { + const api = { + insertCalls: 0, + rebuildCalls: [] as Array<{ kind: string; force: boolean | undefined }>, + async insert() { + api.insertCalls++; + throw new Error("backend index write failed"); + }, + async insertBatch() {}, + async delete() {}, + async search() { + return []; + }, + async rebuildFromShard(args: { kind: string; force?: boolean }) { + api.rebuildCalls.push({ kind: args.kind, force: args.force }); + }, + deleteShardIndexes() {}, + getBackendName() { + return "mock"; + }, + }; + return api as never; +} + +describe("post-commit backend failure repair (Devin/Codex findings on #63)", () => { + let dir: string; + let dbPath: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "backend-repair-")); + dbPath = join(dir, "shard.db"); + shard.dbPath = dbPath; + const db = connectionManager.getConnection(dbPath); + db.run(` + CREATE TABLE memories ( + id TEXT PRIMARY KEY, content TEXT NOT NULL, vector BLOB NOT NULL, tags_vector BLOB, + container_tag TEXT NOT NULL, tags TEXT, type TEXT, created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, metadata TEXT, display_name TEXT, user_name TEXT, + user_email TEXT, project_path TEXT, project_name TEXT, git_repo_url TEXT, + is_pinned INTEGER DEFAULT 0, is_deprecated INTEGER DEFAULT 0, + recency_score REAL DEFAULT 0.5, frequency_score REAL DEFAULT 0.5, + importance_score REAL DEFAULT 0.5, utility_score REAL DEFAULT 0.5, + novelty_score REAL DEFAULT 0.5, confidence_score REAL DEFAULT 0.5, + interference_penalty REAL DEFAULT 0, strength REAL DEFAULT 0.5, + access_count INTEGER DEFAULT 0, last_accessed INTEGER, store_type TEXT, decay_rate REAL, last_decay_at INTEGER + ) + `); + }); + afterEach(() => { + connectionManager.closeAll(); + rmSync(dir, { recursive: true, force: true }); + }); + + it("insertVector with failing backend resolves, persists, and forces a rebuild on next search", async () => { + const backend = makeBackend(); + const vectorSearch = new VectorSearch(backend); + const db = connectionManager.getConnection(dbPath); + + await expect(vectorSearch.insertVector(db, record("a"), shard)).resolves.toBeUndefined(); + expect(backend.insertCalls).toBeGreaterThan(0); + + const rows = db.prepare("SELECT id FROM memories").all() as Array<{ id: string }>; + expect(rows.map((r) => r.id)).toEqual(["a"]); + + // First search: dirty from the failure → force rebuild. + await vectorSearch.searchInShard(shard, new Float32Array([1, 2, 3, 4]), "mem_user_h", 10); + expect(backend.rebuildCalls.some((c) => c.force === true)).toBe(true); + }); + + it("markShardDirty forces a rebuild even when the index was served before", async () => { + const backend = makeBackend(); + const vectorSearch = new VectorSearch(backend); + const db = connectionManager.getConnection(dbPath); + + // A search on a clean shard bootstraps without force. + await vectorSearch.searchInShard(shard, new Float32Array([1, 2, 3, 4]), "mem_user_h", 10); + expect(backend.rebuildCalls).toHaveLength(2); + expect(backend.rebuildCalls.every((c) => c.force !== true)).toBe(true); + + // Migration marks the shard dirty — next search must FORCE. + vectorSearch.markShardDirty(shard); + await vectorSearch.searchInShard(shard, new Float32Array([1, 2, 3, 4]), "mem_user_h", 10); + expect(backend.rebuildCalls.filter((c) => c.force === true).length).toBe(2); + }); +}); From 1ec9af5350191e2e8baadb624727380029f008bb Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:48:01 +0000 Subject: [PATCH 6/7] fix(migration): re-embed updates the live index, regenerates tag vectors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot/Codex P1 + Devin findings on #63: _reEmbedSingleMemory called updateVector without the shard, so the backend-update branch was skipped entirely — a 'successful' migration left any initialized in-memory index serving old embeddings until restart, and left tags_vector NULL (and after a dimension change, old-dims tag blobs would poison a rebuilt tags index). - resolve full ShardInfo per migrated dbPath and pass it through - re-embed the tags text with the exact capture-time prompt shape (Topics: joined tags) when the row stores tags - after a fully successful shard, markShardDirty() — the next search force-replaces initialized indexes with new-dimension data --- docs/CHANGELOG.md | 4 +++- src/services/migration-service.ts | 33 ++++++++++++++++++++++++++++--- tests/reembed-honesty.test.ts | 5 +++++ 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index aee4baa..cd014fa 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -15,7 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Embedding warmup wait is bounded** - \*\*`updateVector` no longer wipes - **The `memories_fts` update trigger is column-scoped** -- **Vector-index writes after a sqlite COMMIT no longer misreport failure** — a failing backend insert used to reject `insertVector`/`updateVector`/`replaceVector` even though the memory was durably persisted (callers retried into duplicates), and the rebuild-dirty flag never actually repaired an initialized USearch index because `rebuildFromShard` skipped initialized indexes. Dirty shards now force a full index rebuild from sqlite on the next search (Devin finding on #63). +- **Vector-index writes after a sqlite COMMIT no longer misreport failure** +- **Re-embed migrations now update the live vector index** — the migration wrote new vectors to sqlite but never touched an initialized in-memory index, so searches served stale old-model/old-dimension results until process restart (Copilot/Codex/Devin finding on #63). Migrated memories also get their tag embeddings re-generated from the stored tags text; after a successful shard both index kinds are force-rebuilt from sqlite on the next search. + — a failing backend insert used to reject `insertVector`/`updateVector`/`replaceVector` even though the memory was durably persisted (callers retried into duplicates), and the rebuild-dirty flag never actually repaired an initialized USearch index because `rebuildFromShard` skipped initialized indexes. Dirty shards now force a full index rebuild from sqlite on the next search (Devin finding on #63). — routine `access_count`/score updates on every search result and decay cycle no longer re-tokenize content, cutting FTS write amplification (Copilot/Codex finding on #63). `tags_vector` when only the content embedding is refreshed\*\* — semantic tag search survives content-only re-embeds (Devin finding on #63). — a slow or hung local-model load no longer stalls the first chat prompt indefinitely; the search path degrades to text-only for that prompt while the model load continues in the background, instead of failing or permanently disabling embeddings. diff --git a/src/services/migration-service.ts b/src/services/migration-service.ts index dea3684..2dff68e 100644 --- a/src/services/migration-service.ts +++ b/src/services/migration-service.ts @@ -4,6 +4,7 @@ import { vectorSearch } from "./sqlite/vector-search.js"; import { embeddingService } from "./embedding.js"; import { CONFIG } from "../config.js"; import { log } from "./logger.js"; +import type { ShardInfo } from "./sqlite/types.js"; export interface DimensionMismatch { needsMigration: boolean; @@ -227,11 +228,23 @@ class MigrationService { processedCount: number, totalMemories: number, shardId: string, - db: ReturnType + db: ReturnType, + shard?: ShardInfo ): Promise<{ success: boolean; processedCount: number }> { try { const vector = await embeddingService.embedWithTimeout(memory.content); - await vectorSearch.updateVector(db, memory.id, vector); + // Re-embed the tags text too: after a dimension change the stored + // tags_vector has old dimensions and would poison a rebuilt index — + // reproducing the exact capture-time embedding text keeps tag search + // consistent (Devin findings on #63). + let tagsVector: Float32Array | undefined; + const tagsText = typeof memory.tags === "string" ? memory.tags.trim() : ""; + if (tagsText) { + tagsVector = await embeddingService.embedWithTimeout( + `Topics: ${tagsText.split(",").join(", ")}` + ); + } + await vectorSearch.updateVector(db, memory.id, vector, shard, tagsVector); const nextCount = processedCount + 1; this.reportProgress({ @@ -263,6 +276,14 @@ class MigrationService { total: totalMemories, }); + // The mismatch list carries dbPaths — resolve full ShardInfo so the + // re-embeds can update the live backend index (R1 finding on #63). + const shardByDbPath = new Map( + [...shardManager.getAllShards("user", ""), ...shardManager.getAllShards("project", "")].map( + (s) => [s.dbPath, s] + ) + ); + let reEmbeddedCount = 0; let processedCount = 0; let shardHadFailures = false; @@ -283,13 +304,15 @@ class MigrationService { const tempMemories = this._backupMemories(memories); let thisShardFailed = false; + const shard = shardByDbPath.get(shardInfo.dbPath); for (const memory of tempMemories) { const result = await this._reEmbedSingleMemory( memory, processedCount, totalMemories, String(shardInfo.shardId), - db + db, + shard ); processedCount = result.processedCount; if (result.success) { @@ -309,6 +332,10 @@ class MigrationService { "embedding_model", CONFIG.embeddingModel, ]); + // The dims may have changed: any initialized in-memory index + // holds old-dimension vectors. Force a rebuild from sqlite on + // next search — the live index must not go stale until restart. + if (shard) vectorSearch.markShardDirty(shard); } else { log("Migration: keeping original shard due to re-embedding failures", { shardId: shardInfo.shardId, diff --git a/tests/reembed-honesty.test.ts b/tests/reembed-honesty.test.ts index 92e4ed1..0846536 100644 --- a/tests/reembed-honesty.test.ts +++ b/tests/reembed-honesty.test.ts @@ -26,6 +26,7 @@ import { CONFIG } from "../src/config.js"; import { connectionManager } from "../src/services/sqlite/connection-manager.js"; import { shardManager } from "../src/services/sqlite/shard-manager.js"; import { migrationService } from "../src/services/migration-service.js"; +import { vectorSearch } from "../src/services/sqlite/vector-search.js"; const NEW_DIMS = 8; const OLD_DIMS = 4; @@ -145,11 +146,15 @@ describe("re-embed honesty", () => { it("updates vectors in place and reports success", async () => { embedMock.embedWithTimeout.mockResolvedValue(new Float32Array(NEW_DIMS).fill(0.5)); + const markDirty = vi.spyOn(vectorSearch, "markShardDirty"); const result = await migrationService.migrateToNewModel("re-embed"); expect(result.success).toBe(true); expect(result.reEmbeddedMemories).toBe(2); expect(shardManager.deleteShard).not.toHaveBeenCalled(); expect(shardManager.getWriteShard).not.toHaveBeenCalled(); + // Live index must be invalidated — searches after a "successful" + // migration may not serve stale old-dimension vectors until restart. + expect(markDirty).toHaveBeenCalledWith(expect.objectContaining({ dbPath })); const db = connectionManager.getConnection(dbPath); const rows = db.prepare("SELECT id, vector FROM memories ORDER BY id").all() as Array<{ From 146e4a0398f44ddf2e12db576be02d7a02996599 Mon Sep 17 00:00:00 2001 From: ZeR020 <88128532+ZeR020@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:50:14 +0000 Subject: [PATCH 7/7] fix(web): explicit allowed Hosts, IPv6-safe parse, phrase-quoted FTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 + Copilot + Devin findings on #64: - webServerHost doubled as bind address AND Host-allowlist entry, so '0.0.0.0' binds rejected every remote client (403 on the whole dashboard) and the documented proxy workaround silently changed the listen interface. webServerAllowedHosts (default []) now lists extra accepted Host names; bind semantics of webServerHost unchanged. - unbracketed IPv6 Host literals (2001:db8::1) were misparsed as host:port ('host 2001:db8:, port 1'); multi-colon strings are now treated as the whole hostname. - safeFtsQuery stripped a fixed punctuation list — apostrophes, periods, @ and more still reached FTS5 as invalid MATCH syntax and returned silently empty results. Token quoting ("tok") neutralizes operators and keeps punctuation matchable; shared helper serves both the memory (searchFTS5) and transcript paths. --- docs/CHANGELOG.md | 5 ++- docs/CONFIGURATION.md | 17 +++++----- src/config.ts | 3 ++ src/index.ts | 1 + src/services/sqlite/sqlite-bootstrap.ts | 21 ++++++++++++ src/services/sqlite/transcript-manager.ts | 8 ++--- src/services/sqlite/vector-search.ts | 8 ++--- src/services/web-server.ts | 11 +++++-- tests/transcript-fts-sanitize.test.ts | 20 ++++++++++++ tests/web-server-routes.test.ts | 39 +++++++++++++++++++++++ 10 files changed, 110 insertions(+), 23 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index cd014fa..038a32d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -16,7 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - \*\*`updateVector` no longer wipes - **The `memories_fts` update trigger is column-scoped** - **Vector-index writes after a sqlite COMMIT no longer misreport failure** -- **Re-embed migrations now update the live vector index** — the migration wrote new vectors to sqlite but never touched an initialized in-memory index, so searches served stale old-model/old-dimension results until process restart (Copilot/Codex/Devin finding on #63). Migrated memories also get their tag embeddings re-generated from the stored tags text; after a successful shard both index kinds are force-rebuilt from sqlite on the next search. +- **Re-embed migrations now update the live vector index** +- **Keyword search no longer returns silent empty results for punctuated queries** — `don't`, `foo.js`, `email@example.com`, and reserved words like `AND` previously produced invalid FTS5 MATCH syntax that degraded to an empty result set. Tokens are now quoted as phrases (both the memory and transcript search paths) and unbracketed IPv6 Host headers parse correctly (Devin/Copilot findings on #64). +- **New `webServerAllowedHosts` setting** — binding the dashboard remotely (`webServerHost: "0.0.0.0"`) or behind a reverse proxy previously rejected every remote client's Host header (Codex P1 on #64); the documented workaround also changed the listen interface. The bind address and the accepted Host names are now configured separately, keeping the DNS-rebinding protection for loopback defaults. + — the migration wrote new vectors to sqlite but never touched an initialized in-memory index, so searches served stale old-model/old-dimension results until process restart (Copilot/Codex/Devin finding on #63). Migrated memories also get their tag embeddings re-generated from the stored tags text; after a successful shard both index kinds are force-rebuilt from sqlite on the next search. — a failing backend insert used to reject `insertVector`/`updateVector`/`replaceVector` even though the memory was durably persisted (callers retried into duplicates), and the rebuild-dirty flag never actually repaired an initialized USearch index because `rebuildFromShard` skipped initialized indexes. Dirty shards now force a full index rebuild from sqlite on the next search (Devin finding on #63). — routine `access_count`/score updates on every search result and decay cycle no longer re-tokenize content, cutting FTS write amplification (Copilot/Codex finding on #63). `tags_vector` when only the content embedding is refreshed\*\* — semantic tag search survives content-only re-embeds (Devin finding on #63). diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index e03350a..0d13b55 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -139,14 +139,15 @@ Auto-capture observes chat exchanges and automatically extracts memorable inform ## Web UI Settings -| Setting | Type | Default | Description | -| ------------------ | --------- | ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `webServerEnabled` | `boolean` | `true` | Enable the management web UI. | -| `webServerPort` | `number` | `4747` | Port for the web UI server. | -| `webServerHost` | `string` | `"127.0.0.1"` | Host binding for the web server. Defaults to loopback for security. **`webServerApiKey` is required if binding to a non-loopback address.** | -| `webServerApiKey` | `string` | — | API key for authenticating web UI requests. Required when `webServerHost` is not a loopback address (`127.0.0.1`, `localhost`, `::1`). Value is used as-is (no secret resolution). | - -Requests are accepted only when the `Host` header is loopback (`127.0.0.1`, `localhost`, `[::1]`) or the configured `webServerHost` (hostname compared case-insensitively, port ignored). If you reverse-proxy the dashboard, set `webServerHost` to the proxy hostname. +| Setting | Type | Default | Description | +| ----------------------- | ---------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `webServerEnabled` | `boolean` | `true` | Enable the management web UI. | +| `webServerPort` | `number` | `4747` | Port for the web UI server. | +| `webServerHost` | `string` | `"127.0.0.1"` | Host binding for the web server. Defaults to loopback for security. **`webServerApiKey` is required if binding to a non-loopback address.** | +| `webServerApiKey` | `string` | — | API key for authenticating web UI requests. Required when `webServerHost` is not a loopback address (`127.0.0.1`, `localhost`, `::1`). Value is used as-is (no secret resolution). | +| `webServerAllowedHosts` | `string[]` | `[]` | Extra hostnames accepted in the `Host` header. Add the public hostname (or LAN IP) clients use to reach the dashboard when binding to `0.0.0.0`/an interface address or when running behind a reverse proxy. | + +Requests are accepted only when the `Host` header is loopback (`127.0.0.1`, `localhost`, `[::1]`), the configured `webServerHost`, or one of `webServerAllowedHosts`. **Binding remotely or behind a reverse proxy:** keep `webServerHost` as the address to _bind_ (e.g. `127.0.0.1` behind a local proxy, `0.0.0.0` for LAN access) and list the hostname clients send in `Host` under `webServerAllowedHosts`. ## Vector Search Settings diff --git a/src/config.ts b/src/config.ts index 69f9203..12aacd5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -56,6 +56,7 @@ export const OpenCodeMemConfigSchema = z.object({ webServerEnabled: z.boolean().optional(), webServerPort: z.number().positive().max(65535).optional(), webServerHost: z.string().optional(), + webServerAllowedHosts: z.array(z.string()).optional(), webServerApiKey: z.string().optional(), maxVectorsPerShard: z.number().positive().optional(), autoCleanupEnabled: z.boolean().optional(), @@ -165,6 +166,7 @@ const DEFAULTS: Partial = { webServerEnabled: true, webServerPort: 4747, webServerHost: "127.0.0.1", + webServerAllowedHosts: [], maxVectorsPerShard: 50000, autoCleanupEnabled: true, autoCleanupRetentionDays: 30, @@ -351,6 +353,7 @@ function mergeConfigWithDefaults(fileConfig: OpenCodeMemConfig) { webServerEnabled: cfg.webServerEnabled ?? defaults.webServerEnabled, webServerPort: cfg.webServerPort ?? defaults.webServerPort, webServerHost: cfg.webServerHost ?? defaults.webServerHost, + webServerAllowedHosts: cfg.webServerAllowedHosts ?? defaults.webServerAllowedHosts, webServerApiKey: cfg.webServerApiKey, maxVectorsPerShard: cfg.maxVectorsPerShard ?? defaults.maxVectorsPerShard, autoCleanupEnabled: cfg.autoCleanupEnabled ?? defaults.autoCleanupEnabled, diff --git a/src/index.ts b/src/index.ts index 58e0660..fd39cbe 100644 --- a/src/index.ts +++ b/src/index.ts @@ -221,6 +221,7 @@ export const OpenCodeMemPlugin: Plugin = async (ctx: PluginInput) => { host: CONFIG.webServerHost, enabled: CONFIG.webServerEnabled, apiKey: CONFIG.webServerApiKey, + allowedHosts: CONFIG.webServerAllowedHosts, }) .then(async (server) => { // Disposed before listening completed — do not resurrect state. diff --git a/src/services/sqlite/sqlite-bootstrap.ts b/src/services/sqlite/sqlite-bootstrap.ts index 08961e5..ba22e6a 100644 --- a/src/services/sqlite/sqlite-bootstrap.ts +++ b/src/services/sqlite/sqlite-bootstrap.ts @@ -96,6 +96,27 @@ const wrapStatement = (stmt: RawStatement): Statement => ({ all: (...params: unknown[]) => stmt.all(...normalizeParams(params)), }); +/** + * FTS5-safe query: quotes each whitespace-separated token as a phrase so + * punctuation (don't, foo.js, a@b.com) never produces invalid MATCH + * syntax and reserved words (AND/OR/NOT/NEAR) lose operator meaning. + * Quotes and glob chars are stripped — tokens cannot escape their phrase. + * Returns "" when nothing survives sanitization. + */ +export function toSafeFtsQuery(query: string, maxLength: number = 500): string { + const tokens = query + .replace(/["*]/g, " ") + .split(/\s+/) + .filter((t) => t.length > 0) + .slice(0, 100); + let safe = tokens.map((t) => `"${t}"`).join(" "); + if (safe.length > maxLength) { + safe = safe.slice(0, maxLength); + const boundary = safe.lastIndexOf('"'); + safe = boundary > 0 ? safe.slice(0, boundary + 1) : ""; + } + return safe; +} class SqliteDatabase implements Database { protected readonly db: RawDatabase; diff --git a/src/services/sqlite/transcript-manager.ts b/src/services/sqlite/transcript-manager.ts index d33dff6..1cd910a 100644 --- a/src/services/sqlite/transcript-manager.ts +++ b/src/services/sqlite/transcript-manager.ts @@ -1,4 +1,4 @@ -import { type Database } from "./sqlite-bootstrap.js"; +import { toSafeFtsQuery, type Database } from "./sqlite-bootstrap.js"; import { randomBytes } from "node:crypto"; import { existsSync, mkdirSync } from "node:fs"; import { join, dirname } from "node:path"; @@ -175,11 +175,7 @@ export class TranscriptManager { ): { transcripts: TranscriptRecord[]; total: number } { if (!CONFIG.transcriptStorage.enabled) return { transcripts: [], total: 0 }; - const safeFtsQuery = query - .replace(/[*^:\-+?()"]/g, " ") - .replace(/\s+/g, " ") - .trim() - .slice(0, 500); + const safeFtsQuery = toSafeFtsQuery(query); if (safeFtsQuery.length === 0) return { transcripts: [], total: 0 }; try { diff --git a/src/services/sqlite/vector-search.ts b/src/services/sqlite/vector-search.ts index a6b56cd..3df644f 100644 --- a/src/services/sqlite/vector-search.ts +++ b/src/services/sqlite/vector-search.ts @@ -1,4 +1,4 @@ -import { StmtCache, type Database } from "./sqlite-bootstrap.js"; +import { StmtCache, toSafeFtsQuery, type Database } from "./sqlite-bootstrap.js"; import { connectionManager } from "./connection-manager.js"; import { log, warn } from "../logger.js"; import { CONFIG } from "../../config.js"; @@ -252,11 +252,7 @@ export class VectorSearch { private searchFTS5(db: Database, queryText: string | undefined, limit: number): string[] { if (!queryText || queryText.length === 0) return []; - const safeFtsQuery = queryText - .replace(/[*^:\-+?()"]/g, " ") - .replace(/\s+/g, " ") - .trim() - .slice(0, 500); + const safeFtsQuery = toSafeFtsQuery(queryText); if (safeFtsQuery.length === 0) return []; diff --git a/src/services/web-server.ts b/src/services/web-server.ts index ef9d0a9..1c9c958 100644 --- a/src/services/web-server.ts +++ b/src/services/web-server.ts @@ -67,6 +67,8 @@ interface WebServerConfig { host: string; enabled: boolean; apiKey?: string; + /** Extra hostnames accepted in the Host header when binding remotely or behind a proxy. */ + allowedHosts?: string[]; } type RedactedValue = @@ -90,6 +92,9 @@ function hostnameFromHostHeader(raw: string): string { const end = host.indexOf("]"); if (end !== -1) return host.slice(1, end); } + // Unbracketed IPv6 literals (multiple colons) are the whole hostname — + // splitting on the last colon misclassifies 2001:db8::1 as host:port. + if ((host.match(/:/g) ?? []).length > 1) return host; const colon = host.lastIndexOf(":"); if (colon > 0 && /^\d+$/.test(host.slice(colon + 1))) { return host.slice(0, colon); @@ -102,8 +107,10 @@ function isHostAllowed(headers: Headers, config: WebServerConfig): boolean { if (!raw) return false; const hostname = hostnameFromHostHeader(raw); const allowed = new Set(["127.0.0.1", "localhost", "::1"]); - const configured = hostnameFromHostHeader(config.host); - if (configured) allowed.add(configured); + for (const source of [config.host, ...(config.allowedHosts ?? [])]) { + const configured = hostnameFromHostHeader(source); + if (configured) allowed.add(configured); + } if (allowed.has(hostname)) return true; const rawLower = raw.trim().toLowerCase(); if (config.port !== 80 && config.port !== 443) { diff --git a/tests/transcript-fts-sanitize.test.ts b/tests/transcript-fts-sanitize.test.ts index be9f124..2da48e0 100644 --- a/tests/transcript-fts-sanitize.test.ts +++ b/tests/transcript-fts-sanitize.test.ts @@ -38,4 +38,24 @@ describe("transcript FTS query sanitization", () => { expect(junk.transcripts).toEqual([]); expect(junk.total).toBe(0); }); + + it("matches punctuated terms that previously caused silent empty results", () => { + mgr.saveTranscript("sess-2", "/p", [ + { role: "user", content: "fixed the don't panic bug in react.js via email@example.com" }, + ]); + + // Punctuation FTS5 rejects in barewords — quoted phrases must match. + for (const q of ["don't", "react.js", "email@example.com", "panic bug"]) { + const res = mgr.searchTranscripts(q); + expect(res.transcripts.some((t) => t.sessionId === "sess-2")).toBe(true); + } + // Reserved word as bareword was a syntax error — now a valid (empty) phrase query. + expect(mgr.searchTranscripts("AND").total).toBe(0); + }); + + it("truncates very long queries without producing unbalanced quotes", () => { + expect(() => mgr.searchTranscripts("word ".repeat(400))).not.toThrow(); + const res = mgr.searchTranscripts("word ".repeat(400)); + expect(res).toBeTruthy(); + }); }); diff --git a/tests/web-server-routes.test.ts b/tests/web-server-routes.test.ts index 7a0ebc5..41ab0d9 100644 --- a/tests/web-server-routes.test.ts +++ b/tests/web-server-routes.test.ts @@ -205,6 +205,45 @@ describe("WebServer Routes", () => { expect(res.status).toBe(200); }); + it("allows a remote client Host when listed in webServerAllowedHosts", async () => { + server = new WebServer({ + port: 18081, + host: "0.0.0.0", + enabled: true, + apiKey: "secret123", + allowedHosts: ["dashboard.lan.example"], + }); + (serve as any).mockResolvedValue(mockPlatformServer); + await server.start(); + const fetchHandler = (serve as any).mock.calls[0][0].fetch; + const res = await fetchHandler( + new Request("http://dashboard.lan.example:18081/api/health", { + headers: { host: "dashboard.lan.example:18081" }, + }) + ); + expect(res.status).toBe(200); + }); + + it("parses unbracketed IPv6 Host headers as whole hostnames", async () => { + server = new WebServer({ + port: 18082, + host: "2001:db8::5", + enabled: true, + apiKey: "secret123", + }); + (serve as any).mockResolvedValue(mockPlatformServer); + await server.start(); + const fetchHandler = (serve as any).mock.calls[0][0].fetch; + // Browsers bracket IPv6, but raw clients may not — must not be + // misparsed as host "2001:db8:" + port "5". + const res = await fetchHandler( + new Request("http://[2001:db8::5]:18082/api/health", { + headers: { host: "[2001:db8::5]:18082" }, + }) + ); + expect(res.status).toBe(200); + }); + it("allows the configured non-loopback host", async () => { server = new WebServer({ port: 18080,