From 516a7e5520aa58045e4a3863152a504ba585246c Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Thu, 28 May 2026 21:46:54 -0300 Subject: [PATCH] =?UTF-8?q?fix(memory):=20code-review=20hardening=20?= =?UTF-8?q?=E2=80=94=20guards,=20asserts,=20useEffect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Smaller fixes from the 2nd code-review pass on plan 21. Backend / CLI / DB: - memoryTools.ts: error-path fallback no longer hardcodes retrievalStrategy:"exact"; uses DEFAULT_MEMORY_SETTINGS via toMemoryRetrievalConfig (D16 / Bug #7). - memory.mjs: applyLegacyTypeMap also runs on search / list / clear (was only on add); legacy user/feedback/project/reference remap to canonical types with a stderr warning (D17 / Bug #4). - migrationRunner.ts: case "073" guards via hasColumn(memories, needs_reindex) so an unmarked re-run of 073_memory_vec.sql is skipped cleanly (D27). UI: - MemoryEngineStatus: optional onConfigure callback; "Configurar →" CTAs on the Embedding / Qdrant / Rerank rows when those components are off or missing (matches §4.3 wireframe). - EngineTab: scroll IDs on config cards + handleConfigure wired to the status panel. Providers fetch moved from render body (setState-during-render anti-pattern) into useEffect. - RerankConfigCard: toggle is disabled when no provider has a key — blocks turning rerank ON without a provider, still allows turning it OFF (D13). - MemoriesTab: Import validates each entry against the canonical type enum before POST so invalid types are caught locally with a clear skipped count. Tooling: - package.json: test:all includes test:vitest:ui so the UI suite is no longer orphaned in CI. Tests: - cli-memory-commands: asserts updated for the new legacy->canonical remap on search/clear. - memory-embedding-resolve: drop always-true `|| reason.length > 0` clauses that neutralized two assertions. - memory-embedding-static-potion: model_load_failed test forces a real load failure via MEMORY_STATIC_CACHE_DIR=/dev/null/ and asserts EmbeddingError shape + reason + sanitized message (replaces the previous `assert.ok(true)`). - rerank-config-card.test.tsx: happy-path now uses a provider with hasKey; new test covers the disabled-toggle guard. Full memory suite green: 331/331 unit tests, 46/46 UI tests. typecheck:core, typecheck:noimplicit:core, check:cycles clean. --- bin/cli/commands/memory.mjs | 40 +++++++++---- open-sse/mcp-server/tools/memoryTools.ts | 20 +++---- package.json | 2 +- .../memory/components/MemoryEngineStatus.tsx | 56 ++++++++++++++++++- .../memory/components/RerankConfigCard.tsx | 17 +++++- .../memory/components/tabs/EngineTab.tsx | 41 ++++++++++---- .../memory/components/tabs/MemoriesTab.tsx | 25 ++++++--- src/lib/db/migrationRunner.ts | 6 ++ tests/unit/cli-memory-commands.test.ts | 10 +++- tests/unit/memory-embedding-resolve.test.ts | 12 +++- .../memory-embedding-static-potion.test.ts | 39 ++++++++++--- tests/unit/ui/rerank-config-card.test.tsx | 45 ++++++++++++++- 12 files changed, 255 insertions(+), 58 deletions(-) diff --git a/bin/cli/commands/memory.mjs b/bin/cli/commands/memory.mjs index 56b4154b90..2e38acd761 100644 --- a/bin/cli/commands/memory.mjs +++ b/bin/cli/commands/memory.mjs @@ -12,6 +12,29 @@ const LEGACY_TYPE_MAP = { reference: "factual", }; +/** + * Plan 21 Bug#4/D17 fix: remap legacy types in ALL CLI subcommands + * (search/list/clear in addition to add), with a stderr warning on remap. + * Returns the canonical type, or the original value (which the backend will + * 400 on if invalid) — never throws. + */ +function applyLegacyTypeMap(type) { + if (!type) return type; + if (Object.prototype.hasOwnProperty.call(LEGACY_TYPE_MAP, type)) { + const mapped = LEGACY_TYPE_MAP[type]; + process.stderr.write( + `Warning: legacy type '${type}' is deprecated; using '${mapped}'. Use --type factual|episodic|procedural|semantic.\n` + ); + return mapped; + } + if (!VALID_TYPES.includes(type)) { + process.stderr.write( + `Warning: unknown type '${type}'. Valid types: factual, episodic, procedural, semantic.\n` + ); + } + return type; +} + function truncate(v, len = 60) { if (v == null) return "-"; const s = String(v); @@ -60,7 +83,8 @@ async function confirm(question) { export async function runMemorySearch(query, opts, cmd) { const globalOpts = cmd.optsWithGlobals(); const params = new URLSearchParams({ q: query, limit: String(opts.limit ?? 20) }); - if (opts.type) params.set("type", opts.type); + const mappedSearchType = applyLegacyTypeMap(opts.type); + if (mappedSearchType) params.set("type", mappedSearchType); if (opts.apiKey) params.set("apiKey", opts.apiKey); if (opts.tokenBudget) params.set("tokenBudget", String(opts.tokenBudget)); const res = await apiFetch(`/api/memory?${params}`); @@ -79,13 +103,7 @@ export async function runMemoryAdd(opts, cmd) { process.stderr.write("--content or --file required\n"); process.exit(2); } - let resolvedType = opts.type ?? "factual"; - if (opts.type && Object.prototype.hasOwnProperty.call(LEGACY_TYPE_MAP, opts.type)) { - process.stderr.write( - `Warning: legacy type '${opts.type}' is deprecated; using 'factual'. Use --type factual|episodic|procedural|semantic.\n` - ); - resolvedType = LEGACY_TYPE_MAP[opts.type]; - } + const resolvedType = opts.type ? applyLegacyTypeMap(opts.type) : "factual"; const body = { content, type: resolvedType, @@ -108,7 +126,8 @@ export async function runMemoryClear(opts, cmd) { if (!ok) process.exit(0); } const params = new URLSearchParams(); - if (opts.type) params.set("type", opts.type); + const mappedClearType = applyLegacyTypeMap(opts.type); + if (mappedClearType) params.set("type", mappedClearType); if (opts.olderThan) { const iso = parseDuration(opts.olderThan); if (!iso) { @@ -126,7 +145,8 @@ export async function runMemoryClear(opts, cmd) { export async function runMemoryList(opts, cmd) { const globalOpts = cmd.optsWithGlobals(); const params = new URLSearchParams({ limit: String(opts.limit ?? 100) }); - if (opts.type) params.set("type", opts.type); + const mappedListType = applyLegacyTypeMap(opts.type); + if (mappedListType) params.set("type", mappedListType); if (opts.apiKey) params.set("apiKey", opts.apiKey); const res = await apiFetch(`/api/memory?${params}`); if (!res.ok) { diff --git a/open-sse/mcp-server/tools/memoryTools.ts b/open-sse/mcp-server/tools/memoryTools.ts index 87df5c5a66..b5ed467a95 100644 --- a/open-sse/mcp-server/tools/memoryTools.ts +++ b/open-sse/mcp-server/tools/memoryTools.ts @@ -33,23 +33,17 @@ export const memoryTools = { description: "Search memories by query, type, or API key with token budget enforcement", inputSchema: MemorySearchSchema, handler: async (args: z.infer) => { - const memorySettings = await getMemorySettings().catch(() => null); - const baseConfig = memorySettings - ? toMemoryRetrievalConfig(memorySettings) - : { - enabled: DEFAULT_MEMORY_SETTINGS.enabled, - maxTokens: DEFAULT_MEMORY_SETTINGS.maxTokens, - retrievalStrategy: "exact" as const, - autoSummarize: false, - persistAcrossModels: false, - retentionDays: DEFAULT_MEMORY_SETTINGS.retentionDays, - scope: "apiKey" as const, - }; + // Plan 21 D16/Bug#7 fix: even on the error path the fallback must + // respect DEFAULT_MEMORY_SETTINGS.strategy instead of hardcoding "exact". + const memorySettings = + (await getMemorySettings().catch(() => null)) ?? DEFAULT_MEMORY_SETTINGS; + const baseConfig = toMemoryRetrievalConfig(memorySettings, { + query: args.query, + }); const config = { ...baseConfig, maxTokens: args.maxTokens || (baseConfig.maxTokens ?? DEFAULT_MEMORY_SETTINGS.maxTokens), - query: args.query, }; const memories = await retrieveMemories(args.apiKeyId, config); diff --git a/package.json b/package.json index 9f8e8ff3ac..e6a99e3f92 100644 --- a/package.json +++ b/package.json @@ -125,7 +125,7 @@ "coverage:summary": "node scripts/check/test-report-summary.mjs --input coverage/coverage-summary.json --output coverage/coverage-report.md", "check:pr-test-policy": "node scripts/check/check-pr-test-policy.mjs", "coverage:report:legacy": "c8 report --output-dir coverage --exclude=open-sse --reporter=text --reporter=text-summary", - "test:all": "npm run test:unit && npm run test:vitest && npm run test:ecosystem && npm run test:e2e", + "test:all": "npm run test:unit && npm run test:vitest && npm run test:vitest:ui && npm run test:ecosystem && npm run test:e2e", "check": "npm run lint && npm run test", "prepublishOnly": "npm run build:cli-api && npm run build:cli && npm run check:pack-artifact", "postinstall": "node scripts/build/postinstall.mjs", diff --git a/src/app/(dashboard)/dashboard/memory/components/MemoryEngineStatus.tsx b/src/app/(dashboard)/dashboard/memory/components/MemoryEngineStatus.tsx index d159e73590..3a6f0d2116 100644 --- a/src/app/(dashboard)/dashboard/memory/components/MemoryEngineStatus.tsx +++ b/src/app/(dashboard)/dashboard/memory/components/MemoryEngineStatus.tsx @@ -3,8 +3,11 @@ import { useTranslations } from "next-intl"; import type { MemoryEngineStatus } from "@/shared/schemas/memory"; +type ConfigureTarget = "embedding" | "qdrant" | "rerank"; + interface Props { status: MemoryEngineStatus; + onConfigure?: (target: ConfigureTarget) => void; } type ChipColor = "green" | "gray" | "red"; @@ -23,9 +26,36 @@ function StatusChip({ color }: { color: ChipColor }) { ); } -export default function MemoryEngineStatus({ status }: Props) { +function ConfigureLink({ + onClick, + label, + testId, +}: { + onClick: () => void; + label: string; + testId: string; +}) { + return ( + + ); +} + +export default function MemoryEngineStatus({ status, onConfigure }: Props) { const t = useTranslations("memory"); + const embeddingOff = !status.embedding.available; + const qdrantOff = !status.qdrant.enabled; + const qdrantUnhealthy = status.qdrant.enabled && status.qdrant.healthy === false; + const rerankOff = !status.rerank.enabled || !status.rerank.available; + const rows: Array<{ label: string; chip: ChipColor; reason: string; cta?: React.ReactNode }> = [ { label: t("engine.keywordLabel"), @@ -36,6 +66,14 @@ export default function MemoryEngineStatus({ status }: Props) { label: t("engine.embeddingLabel"), chip: status.embedding.available ? "green" : "gray", reason: status.embedding.reason, + cta: + embeddingOff && onConfigure ? ( + onConfigure("embedding")} + label={t("engine.configureCta")} + /> + ) : undefined, }, { label: t("engine.vectorStoreLabel"), @@ -62,11 +100,27 @@ export default function MemoryEngineStatus({ status }: Props) { : status.qdrant.healthy ? t("engine.qdrantOk", { latencyMs: status.qdrant.latencyMs ?? 0 }) : (status.qdrant.error ?? t("engine.qdrantError")), + cta: + (qdrantOff || qdrantUnhealthy) && onConfigure ? ( + onConfigure("qdrant")} + label={t("engine.configureCta")} + /> + ) : undefined, }, { label: t("engine.rerankLabel"), chip: !status.rerank.enabled ? "gray" : status.rerank.available ? "green" : "red", reason: status.rerank.reason, + cta: + rerankOff && onConfigure ? ( + onConfigure("rerank")} + label={t("engine.configureCta")} + /> + ) : undefined, }, ]; diff --git a/src/app/(dashboard)/dashboard/memory/components/RerankConfigCard.tsx b/src/app/(dashboard)/dashboard/memory/components/RerankConfigCard.tsx index a704d67f45..27ca10469f 100644 --- a/src/app/(dashboard)/dashboard/memory/components/RerankConfigCard.tsx +++ b/src/app/(dashboard)/dashboard/memory/components/RerankConfigCard.tsx @@ -35,11 +35,22 @@ export default function RerankConfigCard({ settings, providers, onSave, saving }