You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(plugin): isolate Windows DLLs and resolve DSH memory issues - #2519
Fix problems found while investigating #2495: Windows native model dependencies can conflict with host Sharp/libvips, a recalled memory can abort a DSH V4 turn, malformed import rows are silently reported as skipped, and local model source settings are unavailable or ignored.
Isolate Windows local embeddings in a shared, lazy child process. Transformers/Sharp/ONNX stay outside the host process; requests, cancellation and progress use IPC. Remove inherited host preloads, restart failed children, release the child after the last provider closes, and reclaim idle processes. macOS/Linux retain the in-process path. No DLL replacement, production dependency changes or host-version pinning.
Give both recall and auxiliary host-LLM messages the producer-owned plugin:memos-local-memory source kind accepted by DSH V4.
Validate bundle rows before writing, report duplicates/invalid rows/database failures separately, and roll back a rejected trace together with any synthetic parents. Keep the existing imported/skipped response fields.
Add write-free import previews, explicit preserve/target-profile ownership, and active-profile visibility counts. Existing IDs remain duplicates and sharing scopes are preserved.
Add local embedding mirror/model/cache directories, offline mode, a bounded initial wait, and download/load logs. Explicit mirror configuration overrides HF_ENDPOINT; default model caching lives outside installed package files. Serialize differently configured loads and restore Transformers' shared environment afterward.
Expose these options in Viewer Settings/Import, test unsaved local model settings, and document configuration and import semantics. Remove an accidental real-model dependency from the Hermes summarizer-status unit test.
No production dependency or host peer-range changes. V4 regression tests add the pinned official codec as a dev dependency, align its Cordis dev peer, and pin existing brand/protocol dev peers; both lockfiles are updated. Changes are confined to apps/memos-local-plugin/.
Related Issue (Required): #2495 (partial follow-up). Windows sharp/libvips isolation is now included. Native Windows validation is pending; keep the issue open until both load orders and the original long-session case are confirmed on Windows.
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Documentation update
How Has This Been Tested?
Run from the repository root:
make format
cd apps/memos-local-plugin
npm run lint
npm test
npm run build:package
node node_modules/typescript/bin/tsc --noEmit --strict --target ES2022 --module ES2022 --moduleResolution Bundler --jsx react-jsx --jsxImportSource preact --lib ES2022,DOM --esModuleInterop --skipLibCheck viewer/src/main.tsx
Results: Ruff checks passed (630 Python files unchanged); TypeScript, strict Viewer type checking, and package/Viewer builds passed. Vitest: 190 test files passed; 1638 tests passed, 3 skipped. Repository pre-commit checks passed over the changed files before committing.
Unit Test — structured world-model round trips (including evidence/legacy strings), malformed structure, session/episode ownership conflicts in preview and writes across batches, stable UI/headless preset ownership, explicit-profile isolation, and backup/transactional legacy-profile migration; Windows process isolation without host Transformers import, concurrent response/progress routing, cancellation, crash/restart, startup/IPC failure, invalid vectors and shared-provider cleanup; source attribution, real V4 codec admission/encoding for recall, pre-step inbox batches and host-LLM output, plus rejection of retired sources; local mirror precedence, offline paths, stable cache, shared-environment restoration, serialized loads, progress and timeout/warmup; SQLite import validation, rollback, preview and ownership.
Test Script Or Test Steps (please provide) — On macOS with official DSH 0.2.0-rc.2, use an isolated DSH profile/data home and the logged-in deepseek-flash account. Load this build, import a synthetic deployment-region memory, and ask deployment region without including the stored answer. Recall injected the matching memory, memos_get returned its contents, and the model answered the stored value with a completed turn and no V4 admission error. Restore the original test installation afterward and verify its hash. A real cached local model also returned a 384-dimensional embedding in offline mode.
Pipeline Automated API Test (please provide) — tests/unit/server/http.test.ts verifies import options and invalid-option responses. An isolated HTTP smoke test verified zero writes during preview; one valid, one duplicate and one invalid timestamp row were classified correctly; target ownership made the imported row visible.
Checklist
I have performed a self-review of my own code | 我已自行检查了自己的代码
I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释
I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常
I have created related documentation issue/PR in MemOS-Docs (if applicable) | 我已在 MemOS-Docs 中创建了相关的文档 issue/PR（如果适用） — N/A: local-plugin configuration and import behavior are documented in this package's README.
I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR（如果适用）
I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人
Made sure local checks passed; GitHub CI results are tracked separately on this PR.
Tests have been provided
V4 regression coverage
Bridge tests now use the production recall factory and assert producer-owned source kinds. The published @deepseek-ai/dsh-session-format-v3-to-v4@0.2.0-rc.2 validates both individual messages and the query/recall inbox batch through assertV4RowAdmission and encodeEvent. Host-LLM auxiliary messages pass without form; both old source shapes are rejected. A mutation check restoring kind: "plugin" in the real factory made the admission regression fail with the original error. Tests require no desktop installation or account.
Windows isolation validation
Cross-platform checks completed: real Node and official DSH Electron subprocess smoke tests each returned two concurrent 384-dimensional vectors, with a separate worker PID and no Sharp/libvips/ONNX native libraries added to the parent process. npm pack --dry-run --ignore-scripts confirmed all three worker runtime modules are included.
Native Windows check pending: build the branch and run scripts/verify-windows-dll-isolation.mjs --host-sharp <host sharp package path> [--cache-dir <existing model cache>] [--offline]. The README includes PowerShell instructions for executing with DSH's Electron executable when host Sharp is in ASAR. Each load order runs in a fresh process and verifies both embedding output and real PNG encoding using host Sharp. No native Windows pass is claimed from macOS.
Restart DSH completely before testing the plugin; confirm new and long conversations plus host image processing after local-model initialization. Electron hosts need ELECTRON_RUN_AS_NODE/child_process.fork support; failure does not fall back to loading native model libraries into the host.
QA report follow-up
Address the verified findings from the independent QA report:
Preserve imported world-model structure, including labelled evidence entries and legacy string entries. Reject malformed structures with field diagnostics instead of silently clearing them.
Reject traces whose existing/planned episode belongs to another session or whose parent records belong to another memory owner. Preview tracks accepted parents across batches and applies the same checks without writing records.
Use the configured MemOS profileId consistently for DSH recall, capture, tools and Viewer; agentPreset no longer changes memory ownership. Explicit profiles stay isolated. Older preset-owned records are not silently merged: the packaged migrate-dsh-preset-profile.mjs tool defaults to read-only preview and requires a new SQLite backup before an atomic, explicitly scoped move. Skill-name/constraint conflicts roll back the entire move.
Clarify that the Viewer button Repair embeddings invokes POST /api/v1/embeddings/rebuild with mode: repair.
Validation on the final changes: 190 test files passed; 1638 tests passed, 3 skipped; npm run lint, package/Viewer build, make format (630 Python files unchanged), pre-commit, and npm package-content checks passed. Added regressions failed against the old implementation before the fixes. No schema migration or automatic modification of existing user databases is introduced. Windows native verification remains pending as documented above.
agentKind is hardcoded here while every other call site uses the exported DEEPSEEK_HARNESS_AGENT constant from bridge.ts (bridge.ts:230/533/561/583, index.ts:278/280/304, tools.ts:102). This new file is now the single place that can silently diverge: if the agent kind changes, memory records written through this resolver would carry a different agentKind than those written elsewhere, splitting namespaces. Also note the resulting value↔type import cycle with bridge.ts once the constant is imported here. Suggest importing the constant (import { DEEPSEEK_HARNESS_AGENT } from "./bridge.js" — together with the existing type-only import) and using it.
Now that the preset logic is gone, this local wrapper is a pure passthrough that duplicates DeepSeekHarnessBridge.namespaceFor in bridge.ts. Consider calling deepSeekHarnessNamespace(...) directly at the call sites and removing the wrapper, or keep one shared helper instead of two identical entry points.
The role is hardcoded to "query" for every request, and LocalProcessRequest has no role field, while the parent drops the caller's role on the Windows path (this.isolated.embed(texts, ctx)). Today InProcessLocalEmbeddingProvider ignores the role, so vectors match, but the two paths diverge silently: any future role-aware local model would embed documents as queries on Windows only, degrading recall with no error. Add role: EmbedRole to the embed request and forward it from LocalEmbeddingProvider.embed.
handle is invoked without validating the IPC payload. A null/non-object message, or an embed with a non-array texts, throws synchronously at message.type before the internal try block, so void handle(...) produces an unhandled rejection and the parent's pending entry is never settled (it waits until the child exits or crashes). Guard the shape before dispatch, e.g. if (!message || typeof message !== "object") return; and validate Array.isArray(message.texts).
A config_invalid raised by localSettings (bad remoteHost/HF_ENDPOINT) is flattened into a string here and re-wrapped by the parent as embedding_unavailable, so the host cannot distinguish a configuration mistake from a transient model-load failure (models.ts only surfaces (err as Error).message, and probeLocalEmbedding already prepends “check access to Hugging Face”). Carry the code across the wire — add an optional code?: ErrorCode (or kind) to the error response and rebuild the MemosError with it in receive.
The child inherits the whole parent environment (...process.env) and only strips NODE_OPTIONS/NODE_PATH. That contradicts the protocol's “never forward API keys” comment: HF_TOKEN, provider API keys and unrelated host secrets reach a process that only needs model settings and runs with all stdio ignored. Build an explicit allowlist instead (PATH, HOME/USERPROFILE, TMPDIR/TEMP, HF_HOME, HTTP(S)_PROXY, SystemRoot, ELECTRON_RUN_AS_NODE) and drop everything else.
There is no supervisor-side deadline. Once send succeeds the promise settles only on a result/error frame, an IPC error, or child error/exit. loadTimeoutMs bounds only the load phase inside the child; a wedge during native inference leaves the entry pending forever and the caller blocks with no way to recycle the process. Add a per-request timer (reusing ctx.config.local?.loadTimeoutMs or a dedicated inference timeout) that deletes the pending entry, kills the child via fail, and rejects with embedding_unavailable.
Every embed message starts a concurrent handle against the single shared pipeline — there is no queue or concurrency cap. Two simultaneous searches plus a Settings probe run native feature-extraction in parallel inside one child, spiking RSS and risking a native OOM that takes down all pending requests at once (fail rejects them all). Serialize child-side work through a small queue/semaphore (transformers inference is CPU-bound anyway, so sequencing costs little).
loadQueue is chained to the underlying load promise, so it only advances when pipeline(...) settles. waitForModel rejects just the caller after loadTimeoutMs and leaves the load running; if a stalled native download never settles, the queue stays pending forever and every later call with a different key (offline, remoteHost, cacheDir, modelPath) queues behind it — the timeout message's “configure a mirror or local model directory, then retry” advice cannot work until close(). Bound the load itself (e.g. cap the queue wait, or reset loadQueue/extractorPromise when the timeout fires) so a new configuration can start a fresh load.
remoteHost explicitly accepts http:, so model weights and tokenizer.json can be fetched over plaintext — overridable in transit. Since a remoteHost points at fetchable artifacts (or even a full tokenizer that runs at load time), require https: and keep an explicit opt-out rather than allowing http by default.
close() is not idempotent: after the first call this.isolated is undefined, so a second close() — or a close() on Windows for a provider that never embedded — falls into the else branch and calls inProcess.close(), clearing the module-global extractorPromise/currentKey used by other in-process consumers. Track whether the isolated path was actually acquired (or the mode chosen) instead of re-deriving it from the current field value, or make InProcessLocalEmbeddingProvider.close() a no-op unless it owns the extractor.
The synthesized child-side config hardcodes dimensions: 384 and the cache/OpenRouter flags that live in core/config/defaults.ts — business values duplicated in a second place. InProcessLocalEmbeddingProvider never reads dimensions, and the parent only checks vector count/finiteness, never width, so a model whose width differs from 384 goes unnoticed. Drop dimensions (parent already infers/validates) or forward the host's effective value, and reuse the defaults module for the cache flags.
The contract doesn't say whether imported counts rows that were only previewed. bundle-import.ts increments imported (and visible/hidden) even when dryRun: true, so a caller that treats imported > 0 as "rows were written" will act on rows that were never persisted (index refresh, audit entry, cache invalidation). Please document the dry-run semantics explicitly, or split the counters (e.g. wouldImport vs imported).
kind covers only the four top-level collections, but the import path also synthesizes sessions and episodes parent rows. A failure while writing a synthetic parent (previously swallowed in the old implementation) has no accurate kind to report itself under, so producers must either mislabel it as "trace" or drop the diagnostic. Consider adding "session" | "episode" to the union.
This is a breaking change on a public contract: skipped is redefined as "duplicates + invalid + failed" and four new fields are required on the result object. Existing consumers were not migrated with it — server/routes/migrate.ts still computes traces: Math.max(0, res.imported - skills.length) and reports res.skipped verbatim, so rows rejected as invalid or failed now surface to the user as plain "skipped". Anyone implementing MemoryCore (adapters, test doubles returning { imported, skipped }) also fails to compile. Please update the remaining call sites / implementations in the same change.
These two statements were inserted above the node:crypto builtin, breaking the file's existing grouping (Node builtins first, then agent-contract/*, then ../*). Move them below the import { randomUUID } from "node:crypto"; line so the header block stays consistent.
After moving the import body out, the module-local dedupeTraceIds helper (still defined near line 5316) has no remaining caller in this file — its only use was the episode appendTrace step that moved to bundle-import.ts, which inlines its own new Set(...) dedupe. It is dead code here (noUnusedLocals is false, so it won't fail the build). Remove it, or move it to a shared util if the new module should reuse it instead of duplicating the logic.
The producer identity is hardcoded as string literals in four spots in this file (the kind/plugin members of MemOSMessageSource, the MessageSourceMap key "plugin:memos-local-memory", and again in the factory body), even though DEEPSEEK_HARNESS_PLUGIN = "memos-local-memory" already exists in bridge.ts. The runtime object and the augmentation key must agree with the host-side codec's expectation, so a rename of the plugin constant would leave the V4 attribution (and MessageSourceMap typing) silently out of sync. Prefer deriving the literals from the existing constant (kind: plugin:${typeof DEEPSEEK_HARNESS_PLUGIN}``, plugin: typeof DEEPSEEK_HARNESS_PLUGIN) so there is a single source of truth.
The load timeout default 120_000 is now hardcoded in three places: here, NumberInRange(120_000, …) in schema.ts, and the runtime fallback config.local?.loadTimeoutMs ?? 120_000 in core/embedding/providers/local.ts. If one is tuned (e.g. raised for slow model downloads), the others silently disagree — schema default vs. applied default can diverge. Export a shared constant and reference it from all three sites.
The new local block is the only user-facing embedding sub-section without a doc comment, yet its semantics are non-obvious and differ from the raw value: remoteHost wins over HF_ENDPOINT (empty ⇒ Hugging Face), an empty cacheDir resolves to ~/.cache/memos/models at runtime, modelPath is the local model root containing <model>/, and loadTimeoutMs only bounds the caller's wait (loading continues in the background). Since this schema is also the source of the generated templates/config.*.yaml, documenting it here surfaces the guidance to users.
/**
* Local Transformers.js model source overrides.
* remoteHost HTTP(S) mirror base URL; takes precedence over HF_ENDPOINT.
* Empty for both = Hugging Face.
* modelPath Local model root containing `<model>/` files.
* cacheDir Model cache; empty = `~/.cache/memos/models` (runtime default).
* offline true = use local/cache files only, never download.
* loadTimeoutMs Bounds the caller's wait only; loading may continue in the
* background and warm the next request.
*/
local: Type.Optional(Type.Object({
remoteHost: StringWithDefault(""),
modelPath: StringWithDefault(""),
cacheDir: StringWithDefault(""),
offline: Bool(false),
loadTimeoutMs: NumberInRange(120_000, 1, 3_600_000),
}, { default: {} })),
The doc comment overpromises: bundle-import.ts resolves ownership via ownerFor(), which uses row.ownerAgentKind ?? fallback.ownerAgentKind (and the same for profile/workspace). Rows that carry no ownership fields — legacy exports, hand-written bundles, Hermes-native traces — are therefore stamped with the importing namespace's owner even under ownership: "preserve". That is the opposite of what the comment states and it has a visible consequence: such rows become visible to the active profile rather than staying hidden. Please document the fallback explicitly ("rows without ownership fields inherit the active namespace's owner") so callers pick ownership with the real semantics in mind.
The 50-issue cap is stated, but the result gives no way to tell a clean run from a silently truncated one: issues.length === 50 may mean exactly 50 problems or thousands. Since invalid/failed are counted in full, a consumer that wants to surface "showing first 50 of N" has to compute it from other fields and still cannot see which collections were dropped. Consider adding a issuesTruncated: boolean flag (or a total issue count) so the truncation is observable from the contract itself.
This rejects with ctx.signal.reason directly, while the abort listener a few lines below uses ctx.signal?.reason ?? new DOMException("Aborted", "AbortError"), and local.ts centralizes the same fallback in abortReason(). If reason is ever undefined (custom/polyfilled signals, or AbortSignal-like objects passed through adapters), the caller gets a rejection with undefined instead of an Error, which then flows into asEmbeddingError and produces a meaningless message. Reuse the same fallback helper for consistency.
💡 Suggested Change
Before:
if (ctx.signal?.aborted) return Promise.reject(ctx.signal.reason);
After:
if (ctx.signal?.aborted)
return Promise.reject(
ctx.signal.reason ?? new DOMException("Aborted", "AbortError"),
);
The default model cache path is hardcoded to ~/.cache/memos/models, which bypasses the project's home resolution (resolveHome()/MEMOS_HOME in core/config/paths.ts). Users who relocate the Memos home (or run with a per-agent home) still get weights written to the OS home, and there is now a second, hardcoded notion of "where Memos data lives". Prefer deriving the default from the existing path helper (with the current value as a fallback for hosts that have no home yet).
The allow-list of log levels is duplicated verbatim in local-process-entry.ts (the Object.fromEntries([...]) builder) and here. The two lists must stay in sync with ProviderLogger, but nothing enforces that, so adding/removing a level silently drops frames on one side. Export a shared LOG_LEVELS constant from the protocol module and use it in both places.
cacheDir defaults to an empty string, but core/embedding/providers/local.ts resolves an empty value to ~/.cache/memos/models and always assigns it to env.cacheDir. Before this change env.cacheDir was never touched, so existing installs cache the model wherever Transformers.js put it by default. After upgrading, that location changes, so every existing installation re-downloads the model once — and an install that was working from a pre-populated offline cache now fails with embedding_unavailable until the download succeeds. Suggest detecting the legacy cache directory (or only overriding env.cacheDir when cacheDir is explicitly set) and logging the resolved path at load time so operators can diagnose the move.
remoteHost accepts any string here, while the runtime rejects non-HTTP(S) values (credentials, query, fragment) by throwing MemosError("config_invalid", …) inside localSettings() — i.e. on every embed call instead of once at loadConfig(). Elsewhere this file encodes constraints declaratively (NumberInRange, Type.Union), so a typo'd mirror URL passes config validation and then fails at request time. Consider constraining the field in the schema (e.g. pattern: "^$|^https?://") so loadConfig fails fast; the Settings "Test" path validates against the raw request body and is unaffected.
The lower bound of 1 (millisecond) is degenerate: any value in [1, 999] guarantees the timeout in waitForModel() fires before the model can possibly load, permanently breaking the local provider for that config. The Settings UI already clamps the field to a 1 second floor (Math.max(1, …) * 1000), so the schema should match — use 1_000 as the minimum.
structure is validated to allow legacy plain-string entries, but WorldModelDTO.structure is typed as WorldModelStructureEntryDTO[] ({label, description, evidenceIds}) and insertRow writes those strings straight into WorldModelRow.structure via an as WorldModelRow cast. The cast hides the mismatch, so legacy bundles import "successfully" while every consumer that reads entry.label / entry.description silently gets undefined. Either normalize strings at insert time ({ label: <string>, description: <string> }) so the stored shape always matches the row type, or reject them in validation instead of accepting them.
?? only falls back on null/undefined, so an explicitly nullownerWorkspaceId in the bundle — the normal encoding for "not workspace-scoped" — is silently replaced by the importing namespace's workspace. Under the default ownership: "preserve" this re-scopes rows to the importer's workspace and can hide memories that were globally scoped at export time. Use an explicit presence check so a null in the bundle stays null, e.g. "ownerWorkspaceId" in row ? (row.ownerWorkspaceId as string | null) : fallback.ownerWorkspaceId.
The whole batch body — including validationIssue (pure CPU work over up to 100 rows) and one repos.*.getById duplicate lookup per row — runs inside db.tx, which maps to BEGIN IMMEDIATE and therefore takes the SQLite write lock for the duration. Importing a large bundle blocks every concurrent writer in the process while only validating. Run validation (and ideally the duplicate prefetch) over the batch before opening the transaction, then keep the tx to the insert loop only.
conflictingParent (and the repos.sessions / repos.episodes lookups it performs) sits outside the per-row try, and the outer db.tx has no try/catch. If it throws, the exception escapes the batch callback, rolls back the whole batch and aborts every remaining batch, while result.imported / visible / hidden and planned have already been mutated for the rows earlier in that batch — so the returned counters no longer describe the database, and the caller gets an unhandled exception instead of a per-row issue. Move the conflict check inside the row try (reporting it as an issue), or wrap the batch body so a batch-level failure is recorded rather than propagated.
Validation whitelists public and hub, and insertRow writes dto.share verbatim onto traces, policies, skills, world models and the synthetic episodes. visibilityWhere / isVisibleTo treat hub as visible to every namespace, so any caller that can reach POST /api/v1/import can escalate the local user's memories to cross-profile/hub visibility purely by supplying a share block — the import path becomes the only writer that bypasses the normal share-state transitions. Consider clamping imported share scope (e.g. to private, or to the target namespace) unless an explicit option opts into restoring shared state.
The guard above (!bundle || typeof bundle !== "object") does not reject arrays, so a [...] body reaches importMemoryBundle, which throws MemosError("invalid_argument", ...). That throw bypasses writeError and relies on the dispatcher mapping MemosError — a raw array upload therefore likely surfaces as a 500 rather than the 400 the sibling checks produce. Add Array.isArray(bundle) to the guard (or catch MemosError here) so all invalid-bundle shapes get the same 400 response. The ownership/targetProfileId re-validation below also duplicates what the pipeline already enforces; keeping only the parse/coercion here would avoid the two copies drifting apart.
req.local comes straight from the request body, so the caller controls modelPath, cacheDir and remoteHost for the local probe. localSettings() in core/embedding/providers/local.ts only normalises these paths (resolve() / ~/ expansion) and writes env.localModelPath / env.cacheDir for the transformers pipeline, with no confinement to a plugin-owned root — a request can point the loader at any directory to read as a model and at any directory to write download cache. remoteHost is protocol-checked but still allows any HTTP(S) host, so the probe performs outbound fetches to a caller-chosen mirror. Since /api/v1/models/test is reachable from the loopback-exempt path, this makes the endpoint a local-file-read/write and SSRF primitive rather than a pure "test" call.
Suggest only forwarding the benign fields and taking filesystem locations from the saved config (or a fixed default):
TestRequest.local is typed as EmbeddingConfig["local"] but parseJson only JSON-parses the body — nothing checks the shape. A caller can send local: "...", local: [] or local: { loadTimeoutMs: "60000" }; loadTimeoutMs then reaches setTimeout(..., "60000") in waitForModel (implicitly coerced, no error surfaced) and string paths hit localPath()/resolve() with non-string input. Validate the object here and return the existing invalid_argument 400 rather than letting malformed values fail deep inside provider construction.
The catch branch sets an error status but never clears preview. After a failed re-preview (network error, server 400 from the ownership/targetProfileId validation, or an aborted request) the previously rendered preview survives, so the issue list is stale and the “Import valid rows” button stays enabled (!preview is false, preview.imported > 0) — the user can confirm a real import based on counts and a target profile that no longer match the file/options on screen. Clear the preview when a preview request fails (keep result intact so a failed re-import does not hide an earlier success).
The file input value is never reset, so picking the same bundle again (e.g. after editing the file on disk or after a failed import) fires no change event: setFile/run(f, true) never execute and the UI keeps showing the previous file with no new preview. Reset e.target.value after reading the selection so a re-selection always re-triggers onChange.
💡 Suggested Change
Before:
if (f) { setFile(f); resetPreview(); void run(f, true); }
After:
if (f) { (e.target as HTMLInputElement).value = ""; setFile(f); resetPreview(); void run(f, true); }
(preview ?? result)!.issues relies on a non-null assertion after the optional-chained length check. issues is non-optional in MemoryImportResult, so the guard is only needed for preview ?? result; hoisting it into a local (const report = preview ?? result;) removes the ! and keeps future guard changes type-checked instead of turning into a runtime crash.
run()'s changes count is discarded, so an apply against the wrong --from, a wrong AGENT literal, or a profile id that never existed completes with dryRun: false and exit code 0 after rewriting nothing. For a repair script that operators may follow by deleting the legacy profile, that silent no-op is the failure mode worth blocking. Accumulate the changes across tables and fail when the total is 0 (the operator can then re-check --from).
💡 Suggested Change
Before:
db.prepare(
`UPDATE ${quote(name)} SET owner_profile_id=? WHERE owner_agent_kind=? AND owner_profile_id=?`,
).run(to, AGENT, from);
After:
changed += db.prepare(
`UPDATE ${quote(name)} SET owner_profile_id=? WHERE owner_agent_kind=? AND owner_profile_id=?`,
).run(to, AGENT, from).changes;
The preview only counts rows owned by from. A target profile that already holds rows is invisible here, yet it is what decides whether the generic UPDATE hits uq_skills_owner_name (skills) and merges rows into an existing profile — the exact outcome the comment says must never happen silently. Including the destination counts in the dry-run result (same query with to) makes that decision visible before the backup is taken.
💡 Suggested Change
Before:
if (!apply) return { from, to, dryRun: true, counts };
After:
if (!apply) {
const targetCounts = Object.fromEntries(
tables.map(({ name }) => [
name,
db
.prepare(
`SELECT COUNT(*) AS n FROM ${quote(name)} WHERE owner_agent_kind=? AND owner_profile_id=?`,
)
.get(AGENT, to).n,
]),
);
return { from, to, dryRun: true, counts, targetCounts };
}
The reserved backup file is never cleaned up: if db.backup() throws (disk full, locked source), a zero-byte file stays at --backup, and a retry with the same path dies on EEXIST from this openSync instead of a message about the real failure. Wrap the backup in try/catch and unlinkSync(backupPath) before rethrowing.
On a failing child, execFile rejects and this code propagates before result exists, so that child's stdout/stderr are never written and the failing --order label is lost. Since this script exists purely as a diagnostic gate, catch the rejection and forward error.stdout/error.stderr (labelled with sequence) before rethrowing. Note also that a plugin-first failure prevents host-first from ever running, so a single run only reports one order.
--model is configurable but the dimensionality is not: dimensions and both assert.equal(..., 384) checks are hardcoded to all-MiniLM-L6-v2, so running the documented command with any other model reports an isolation failure that is really a dimension mismatch. Either derive the expected dimension from the vectors/--dimensions, or drop the --model flag.
Worker detection depends on the literal log message "process.request" and data.pid (see core/embedding/providers/local-process.ts:163). If that event is renamed or its payload changes, workerPid stays undefined and the failure reads "Local model must run in a separate process", which points at isolation rather than at the harness. Prefer a stable signal (e.g. assert on the child process handle the provider exposes, or fail with a distinct message when the log event was never observed).
46. apps/memos-local-plugin/package.json (L42)
The published README (auto-included by npm) instructs users to run scripts/verify-windows-dll-isolation.mjs, but only migrate-dsh-preset-profile.mjs was added to files, so the documented command is unavailable from the installed tarball. Either ship the verifier too, or mark it in the README as a repo-only check to keep the asymmetry intentional.
47. apps/memos-local-plugin/package.json (L139)
The dev pin was narrowed to ~4.0.4 (resolving 4.0.5) in this change, but the unchanged peerDependencies entry still advertises ^4.0.1. A host on cordis 4.0.1–4.0.3 satisfies the peer range while never being exercised by this repo's tests, so if the narrowing reflects a real minimum the peer range should be tightened to match; otherwise the reason for narrowing is worth a comment so the two ranges don't drift.
Owner fields are only checked with typeof === "string", so an empty string (ownerAgentKind: "") passes validation and is persisted verbatim as owner_agent_kind. That value is then interpreted inconsistently: isVisibleTo treats a falsy ownerAgentKind as "visible to every namespace" (if (!row.ownerAgentKind ...) return true), while visibilityWhere matches nothing for '' (not NULL, not 'unknown', no share scope), making the row invisible to every SQL query. Result: the dry-run visible/hidden counters (and therefore the "Import valid rows" gating) can disagree with what retrieval actually returns. ownerProfileId has the same problem via COALESCE(owner_profile_id, 'default'). Reject blank strings for the three owner fields here, or normalize them before writing.
ids.add(id) runs only after a successful insert, so a row whose insert fails is never registered in the per-collection de-dupe set. If the same id appears again later in the bundle, it is retried, hits the same constraint, and produces a second write_failed issue — failed is inflated per repetition and the row is never classified as a duplicate. Register the id before attempting the insert (or in the catch) so repeated rows collapse to a single failure.
The catch below unconditionally rewraps every failure into new Error("Local embedding model download/load failed; check access to Hugging Face: …"). That was accurate when the probe could only fail on download, but the newly forwarded local settings add failure modes whose message is then contradicted by the prefix: localSettings() throws MemosError("config_invalid", "…remoteHost/HF_ENDPOINT must be an HTTP(S) base URL…"), waitForModel throws embedding_unavailable for loadTimeoutMs, and offline: true failures have nothing to do with Hugging Face reachability. Rewrapping also drops the MemosError.code, so the UI can no longer distinguish a bad config from a network problem. Re-throw MemosError unchanged (or branch the hint on offline/local.modelPath) instead of wrapping blindly.
Adding local as the third positional parameter forces callers to pad the middle argument (probeLocalEmbedding(model, undefined, req.local) at line ~289) and makes the exported signature easy to misuse — the existing unit tests call the two-argument form and silently skip the settings. A single options object (probeLocalEmbedding(model, { create, local })) keeps call sites readable and extensible without reordering arguments again.
Every value here is untranslated: issue.message comes from the server in English, and issue.kind / issue.code / issue.field are raw enum/property names (worldModel, invalid_field, ownerProfileId). In the zh locale the rest of this card is localized, so users get a Chinese status line followed by English diagnostics. Map issue.code to an i18n key (falling back to issue.message), and render a localized label for kind instead of the internal identifier.
New static inline style attributes are added here and on the wrapper div class="vstack" style="gap:var(--sp-3)". These gaps are fixed layout values, not dynamic styles, so they belong in the stylesheet (e.g. a modifier class) rather than inline — inline styles bypass theming and CSP style-src policies.
The row is already filtered to owner_agent_kind = AGENT above, so gating the namespace rewrite on meta.namespace.agentKind === AGENT adds no safety but does add a partial-rewrite path: if a session/episode was written with namespace.profileId but no namespace.agentKind (e.g. meta built from caller hints rather than namespaceMeta()), owner_profile_id and meta.ownerProfileId become to while meta.namespace.profileId stays from. On reload namespaceFromHints() prefers the embedded namespace.profileId, so the stale id survives and future captures can recreate rows under the legacy profile. Gate only on profileId === from.
💡 Suggested Change
Before:
if (
meta.namespace?.agentKind === AGENT &&
meta.namespace.profileId === from
) {
On Windows import.meta.url preserves the on-disk casing while resolve(process.argv[1]) reflects whatever the operator typed, and symlinked/junctioned paths differ too — the comparison can be false and the process then exits 0 having printed nothing. For a repair tool that is indistinguishable from a successful no-op run. Compare case-insensitively on win32 (or fall back to comparing realpath), and/or emit a message when the module is loaded but the CLI branch is not entered.
56. apps/memos-local-plugin/package.json (L144)
Every other @deepseek-ai/dsh-* package here is pinned to 0.1.0-rc.6, but this codec is 0.2.0-rc.2, and its dsh-session-format-v1-to-v2 dependency requires dsh-brand/dsh-typert-protocol at 0.2.0-rc.2 — which does not satisfy the ^0.1.0-rc.6 range, so the lockfiles resolve a second, nested copy (see package-lock.jsonnode_modules/@deepseek-ai/dsh-session-format-v1-to-v2/node_modules/@deepseek-ai/dsh-brand). Two live instances of brand/typert-protocol singletons alongside cordis plugins is a classic source of instanceof/service-registry mismatches in tests. Either move the dsh family to 0.2.0-rc.2 together or use a codec release aligned with 0.1.0-rc.6.
The timeout input clamps and rewrites the value on every keystroke, so the field can never be cleared: deleting the contents makes Number("") || 120 evaluate to 120, the input immediately re-renders as 120, and the caret jumps to the end. Typing a multi-digit value from empty therefore often lands on a different number than intended. It also ignores step and keeps fractions. Suggest holding the raw text in state (or only normalizing on onBlur) and flooring the result.
ProviderBlock.local duplicates the local shape now declared in core/embedding/types.ts (EmbeddingConfig["local"]) and in core/config/schema.ts. Three copies of the same field list will drift as soon as one gains/loses a key. The viewer already reaches into core for shared types (see components/Markdown.tsx importing core/safety/content), so importing/re-exporting EmbeddingConfig["local"] here would keep the contract single-sourced. Also, 120_000 is the schema default duplicated as a magic number in the render below — worth sharing a named constant.
isLocal is derived from block.provider, but the provider <select> below renders with value={block.provider ?? providers[0]}. When block.provider is undefined (e.g. the initial GET /api/v1/config failed and setConfig({}) was used, so embedding is {}), the select visibly shows local (providers[0]) while isLocal is false: the endpoint/apiKey fields are shown and the new local fields are hidden. Derive isLocal from the same resolved value the select uses so the UI stays consistent.
💡 Suggested Change
Before:
const isLocal = type === "embedding" && block.provider === "local";
Every other boolean in this view is rendered with the existing ToggleSwitch component (settings.telemetry, settings.logging.detailedView, settings.algorithm.*, etc.), and Field lays out its children in a flex-direction:column label — an unstyled native checkbox therefore looks inconsistent with the rest of the settings page and its control sits on its own full-width row. Reuse ToggleSwitch here.
The local probe now blocks server-side for up to local.loadTimeoutMs (default 120s, user-configurable up to 3600s), yet runTest passes no signal to api.post. A local test can therefore leave the button spinning for up to an hour with no way to cancel, and the response arrives after the user has possibly edited the fields. Consider creating an AbortController for the request and exposing a cancel action (cleanup on unmount as well).
Generated by cloud-assistant via Open Code Review.
endxxxx
changed the title
fix(plugin): resolve DSH V4 recall, import and local model issues
fix(plugin): isolate Windows DLLs and resolve DSH memory issues
Oct 26, 2026
The test environment encountered an issue that requires manual attention.
Details: Environment preparation failed before any gating tests executed. Failed scopes: memos_local_plugin Branch:codex/fix-local-plugin-dsh-2495
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Memtensor-AI
added
area:plugin
endxxxx
changed the title
Description
Fix problems found while investigating #2495: Windows native model dependencies can conflict with host Sharp/libvips, a recalled memory can abort a DSH V4 turn, malformed import rows are silently reported as skipped, and local model source settings are unavailable or ignored.
plugin:memos-local-memorysource kind accepted by DSH V4.imported/skippedresponse fields.HF_ENDPOINT; default model caching lives outside installed package files. Serialize differently configured loads and restore Transformers' shared environment afterward.No production dependency or host peer-range changes. V4 regression tests add the pinned official codec as a dev dependency, align its Cordis dev peer, and pin existing brand/protocol dev peers; both lockfiles are updated. Changes are confined to
apps/memos-local-plugin/.Related Issue (Required): #2495 (partial follow-up). Windows sharp/libvips isolation is now included. Native Windows validation is pending; keep the issue open until both load orders and the original long-session case are confirmed on Windows.
Reviewers: @hijzy and @whipser030 (assigned by the repository).
Type of change
How Has This Been Tested?
Run from the repository root:
Results: Ruff checks passed (630 Python files unchanged); TypeScript, strict Viewer type checking, and package/Viewer builds passed. Vitest: 190 test files passed; 1638 tests passed, 3 skipped. Repository pre-commit checks passed over the changed files before committing.
deepseek-flashaccount. Load this build, import a synthetic deployment-region memory, and askdeployment regionwithout including the stored answer. Recall injected the matching memory,memos_getreturned its contents, and the model answered the stored value with a completed turn and no V4 admission error. Restore the original test installation afterward and verify its hash. A real cached local model also returned a 384-dimensional embedding in offline mode.tests/unit/server/http.test.tsverifies import options and invalid-option responses. An isolated HTTP smoke test verified zero writes during preview; one valid, one duplicate and one invalid timestamp row were classified correctly; target ownership made the imported row visible.Checklist
Reviewer Checklist
V4 regression coverage
Bridge tests now use the production recall factory and assert producer-owned source kinds. The published
@deepseek-ai/dsh-session-format-v3-to-v4@0.2.0-rc.2validates both individual messages and the query/recall inbox batch throughassertV4RowAdmissionandencodeEvent. Host-LLM auxiliary messages pass withoutform; both old source shapes are rejected. A mutation check restoringkind: "plugin"in the real factory made the admission regression fail with the original error. Tests require no desktop installation or account.Windows isolation validation
npm pack --dry-run --ignore-scriptsconfirmed all three worker runtime modules are included.scripts/verify-windows-dll-isolation.mjs --host-sharp <host sharp package path> [--cache-dir <existing model cache>] [--offline]. The README includes PowerShell instructions for executing with DSH's Electron executable when host Sharp is in ASAR. Each load order runs in a fresh process and verifies both embedding output and real PNG encoding using host Sharp. No native Windows pass is claimed from macOS.ELECTRON_RUN_AS_NODE/child_process.forksupport; failure does not fall back to loading native model libraries into the host.QA report follow-up
Address the verified findings from the independent QA report:
structure, including labelled evidence entries and legacy string entries. Reject malformed structures with field diagnostics instead of silently clearing them.profileIdconsistently for DSH recall, capture, tools and Viewer;agentPresetno longer changes memory ownership. Explicit profiles stay isolated. Older preset-owned records are not silently merged: the packagedmigrate-dsh-preset-profile.mjstool defaults to read-only preview and requires a new SQLite backup before an atomic, explicitly scoped move. Skill-name/constraint conflicts roll back the entire move.POST /api/v1/embeddings/rebuildwithmode: repair.Validation on the final changes: 190 test files passed; 1638 tests passed, 3 skipped;
npm run lint, package/Viewer build,make format(630 Python files unchanged), pre-commit, and npm package-content checks passed. Added regressions failed against the old implementation before the fixes. No schema migration or automatic modification of existing user databases is introduced. Windows native verification remains pending as documented above.