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.
12 matrix legs run native installs/builds and a vitest process with no timeout-minutes at job or step level, combined with fail-fast: false — a hung npm ci/native smoke/vitest run occupies runners indefinitely and blocks the whole matrix. Add a job-level timeout (and optionally per-step timeouts on the install/smoke steps).
Every leg re-downloads the full dependency tree plus native prebuilt binaries (3 OS × 2 Node × 2 managers) with no caching. actions/setup-node is invoked without cache:, so this workflow is one of the most expensive PR triggers. Enable lockfile-keyed caching, e.g. with: { node-version: ..., cache: 'npm' } for the npm leg and cache: 'pnpm' for the pnpm leg.
This new PR-triggered workflow declares no concurrency group, so every push to a PR starts another 12-leg run in parallel with the previous one. Add workflow-level concurrency keyed on the PR ref with cancel-in-progress: true (as done in the other release workflows in this repo).
Also note this workflow pins actions/checkout@v4/actions/setup-node@v4 by tag, while every other workflow in this repo pins third-party and first-party actions to full commit SHAs; align with that convention.
Matrix gap: macos-latest is an arm64 runner, so the darwin-x64 native path is never exercised here, even though the publish workflow still builds and ships a darwin-x64 prebuild (and the Rosetta rebuild step was just removed from it). A regression in the x64 macOS binding/prebuild layout would only surface at release time. Consider adding an explicit macos-15-intel/darwin-x64 leg (or an include: entry with an arch/os pair) so the install contract covers every platform the publisher ships.
This now depends on an internal layout of the installed tarball (prebuilds/<platform>.node, flat file named after the platform). Since better-sqlite3 install scripts are denied (package.json#allowScripts["better-sqlite3"]: false, gypfile: false in the lock), there is no longer any build/Release/better_sqlite3.node fallback, so if v13.0.3 ships the prebuildify directory layout (prebuilds/<platform>-<arch>/better_sqlite3.node) or a different flat name, cp fails inside bash -euo pipefail and the whole release matrix leg goes red. Also note nothing in this repo consumes apps/memos-local-plugin/prebuilds/* (install.sh/install.ps1 and the packaged tarball files list never reference it), so please confirm these artifacts are still needed at all — otherwise the safer fix is to drop the collect/upload/organize steps instead of depending on an undocumented path.
The previous flow asserted test -s prebuilds/linux-x64/better_sqlite3.node here before publishing; that assertion is now gone. The only remaining existence check is the cp in build-prebuilds, and the downloaded aggregate in apps/memos-local-plugin/prebuilds is never validated, so an empty/incomplete prebuilds directory would silently be shipped. Keep an explicit non-empty check for every expected platform (e.g. for p in darwin-arm64 darwin-x64 linux-x64 win32-x64; do test -s prebuilds/$p/better_sqlite3.node; done) after the prebuilds are organized.
These new steps run npm run lock:check / npm run verify:native against the checked-out package source (guard-legacy-publish.outputs.package_source_sha), but those npm scripts (and scripts/normalize-sqlite-lock.cjs, scripts/verify-native-runtime.cjs) only exist from this change onward. The guard still accepts older ancestor commits and paired metadata-only child commits, so publishing from such a ref now fails with Missing script: "lock:check" (or a missing file) instead of publishing. Either gate these calls on the script being present, or explicitly reject package sources that predate the new scripts with a clear ::error:: message.
The plugin identity "memos-local-memory" is now repeated three times in this file (interface field, augmentation key and factory) and is also defined as DEEPSEEK_HARNESS_PLUGIN in bridge.ts. The kind value is derived from it (plugin: + name), so these literals must stay in sync with the exported constant. Deriving both values from a single constant keeps the source kind and the augmentation key consistent after a rename.
The agent kind is hardcoded here as the literal "deepseek-harness" while bridge.ts and tools.ts still use the exported DEEPSEEK_HARNESS_AGENT constant for the same identifier. Once this helper is the single namespace source, any future rename of that constant will silently produce a different agentKind for DSH records written through this path, splitting the namespace. Prefer importing the constant (or moving the identity constants into a small shared module to avoid a bridge.ts <-> namespace.ts runtime cycle).
The message now states the bridge runtime needs Node.js >= 22, but this script still never enforces it and its fallback path (when node_modules is absent) runs a plain npm install --no-audit --no-fund --prefer-offline — without --legacy-peer-deps, --strict-allow-scripts or --engine-strict. Run standalone on a Node 20 host, it would install an unsupported runtime and fail later with a cryptic error instead of the clear message. Add the same >= 22 check and the hardened flags used by install.sh so both entry points behave identically.
The newly added embedding.local block has no field descriptions, even though this schema is emitted as JSON Schema for editor autocomplete, and remoteHost is not constrained to an HTTP(S) URL — an invalid value only fails later at embed time (localSettings() throws MemosError("config_invalid")). Consider adding short descriptions and a pattern/format constraint so bad values are rejected at config load rather than on first embedding call.
handle() reads message.type before any validation and outside its own try, while the returned promise is discarded by void. A null/malformed IPC payload (or any throw before the internal try) therefore becomes an unhandled rejection, which terminates the child by default; every in-flight embedding of all providers sharing this process then fails with the generic "process exited" error. Add a payload guard and a terminal catch.
The worker fabricates an EmbeddingConfig with a hardcoded dimensions: 384 (plus openRouter/cache) because only model and local cross IPC. local.modelPath lets users point at any model, so this value silently diverges from the host's config.dimensions, and any future required EmbeddingConfig field has to be remembered here. Forward the dimension with the request (LocalProcessRequest) instead of a magic number.
Error responses carry only a human-readable message, so the MemosError code/details set in the child are lost. localSettings() throws config_invalid for a bad remoteHost/HF_ENDPOINT, but on Windows the host re-wraps every failure as embedding_unavailable (see receive()), so the same misconfiguration is reported with a different code depending on platform, and callers/log sinks that branch on the code can no longer distinguish a config error from a missing model. Carry the code (and optionally details) across IPC and map it back in the host.
embed() has no IPC-level deadline: if the child stays alive but never replies (hung tsx loader, dropped reply, deadlocked native call) this promise never settles. The worker's own loadTimeoutMs only covers model loading, and many callers invoke embedOne()/embed() without a signal (e.g. core/skill/packager.ts, core/experience/feedback-builder.ts, core/retrieval/tier1-skill.ts), so onAbort can never fire. The pending entry, its abort listener and the texts array then leak, and updateLiveness() keeps the process ref'd forever because pending.size > 0. Suggest arming a per-request timer here (cleared inside cleanup(), and also cleared on resolve/reject) that rejects with EMBEDDING_UNAVAILABLE and sends { type: "cancel" }.
Startup failures are collapsed into a generic message: the real cause (missing local-process-entry.js, tsx resolution failure, ENOENT, spawn denied) is discarded, and because stdio is ["ignore","ignore","ignore","ipc"] the child's stderr is dropped too. Users just see "local embeddings unavailable" with no way to diagnose. This matters most for the entry resolution above: the worker is located relative to import.meta.url, so the same silent failure occurs if the plugin is ever bundled instead of emitted as separate files. Keep the cause and log it.
💡 Suggested Change
Before:
} catch {
return Promise.reject(
new MemosError(
"embedding_unavailable",
"Cannot start the isolated local embedding process.",
Raw "embedding_unavailable" literals are used here (four occurrences) and "config_invalid" in local.ts, while the rest of the codebase uses the exported ERROR_CODES contract (embedder.ts uses ERROR_CODES.EMBEDDING_UNAVAILABLE). Prefer the constants so a typo/code change is caught by the compiler.
The log-level allowlist and level→logger forwarding are duplicated between this module and local-process-entry.ts (which also rebuilds a ProviderLogger per request via as unknown as ProviderLogger, hiding signature drift). Export one PROVIDER_LOG_LEVELS constant plus a typed forwardLog/makeLogger helper from local-process-protocol.ts and reuse it on both sides.
users is only decremented by an explicit close() on the provider instance that acquired it, and acquisition happens lazily inside embed(). Callers that drop a provider without closing it (e.g. the local probe in server/routes/models.ts builds a fresh embedder per request and never calls close()) permanently inflate the count, so after the first probe the process can never be reclaimed by the refcount and only the 5-minute idle timer stops it. Consider tying acquisition to the request lifecycle, or document that close() is mandatory for every provider that has embedded.
The whole host environment is copied into the child, even though local-process-protocol.ts states that only model settings should cross the isolation boundary. Every secret the host holds (OPENAI_API_KEY, provider keys, DB/telemetry credentials, MEMOS_* tokens) is therefore readable inside the forked process. Pass a minimal allowlist instead (e.g. PATH, HOME/USERPROFILE, SystemRoot, TEMP, HF_ENDPOINT, HF_TOKEN, proxy vars) so the child only gets what model loading needs.
When a log arrives for a request that is no longer pending (e.g. progress emitted after the caller aborted), it is forwarded to this.lastLog, i.e. whatever caller most recently called embed(). That misattributes the message to an unrelated request, and lastLog keeps a logger (and its scope) alive until close()/idle shutdown. Route such logs to a module-level fallback logger or drop them when the owning call is gone, rather than reusing another call's logger.
Nested ternary is disallowed by the project style/checklist, and the ~/ handling only covers POSIX style (~\\models is left unresolved on Windows, which is the platform this file now special-cases). Use explicit branches.
The ?? -10 sentinel plus + 10 makes the throttle hard to read, and progressByFile.get(file) === progress is already covered by the range check. Simplify to an explicit previous-value comparison so the intent (report every +10%, always report 100%) is obvious.
?? treats an explicit ownerWorkspaceId: null exactly like a missing field, so with ownership: "preserve" a workspace-less row is silently re-homed into the active namespace's workspace — the opposite of the documented "Preserve source ownership" contract. Only fall back when the key is absent.
💡 Suggested Change
Before:
ownerWorkspaceId:
(row.ownerWorkspaceId as string | null) ?? fallback.ownerWorkspaceId,
After:
ownerWorkspaceId:
"ownerWorkspaceId" in row
? (row.ownerWorkspaceId as string | null)
: fallback.ownerWorkspaceId,
This rejects rows whose parent session/episode is owned by legacy rows. core/runtime/namespace.ts deliberately treats owner_agent_kind = 'unknown' (and NULL) as visible to every profile (isVisibleTo / visibilityWhere), and migration 001 defaults the owner columns to 'unknown', but here such a parent is compared with the concrete namespace owner and reported as conflicting_parent, so importing into a pre-ownership session is impossible. Consider only flagging a conflict when the parent owner is a concrete, differing owner (skip 'unknown'/'default' parents), mirroring the visibility rules.
This re-reads the episode row and rewrites the whole trace_ids_json array once per imported trace. For a bundle with thousands of traces belonging to one episode this is O(n²) serialization plus an filterTraceIdsToExisting existence query per trace, all inside the outer write transaction (long write-lock hold). The deleted implementation accumulated addedByEpisode and called appendTrace once per episode per batch — please restore that batching here.
The catch swallows the driver error entirely: only a generic write_failed issue is produced and nothing is logged with the real constraint/column, so failed > 0 results (with issues truncated to 50) are undiagnosable. Log the serialized error (kind/index, no row text) at debug/warn level inside the catch.
validationIssue accepts/validates confidence (and domainTags), but the world-model insert hardcodes confidence: 0.5 and domainTags: [], so a valid bundle value is silently dropped while the validation implies it is preserved. Either persist confidence from the row ((raw as { confidence?: number }).confidence ?? 0.5) or stop validating it, and keep the validated field list and the insert shape in sync.
db.tx() is called inside an outer db.tx() batch transaction. The storage module documents this pattern as unsupported (core/storage/README.md: "Inside a transaction, don't sprinkle more db.tx(...) calls — better-sqlite3 maps outer tx to BEGIN IMMEDIATE and re-entry will throw") and provides withSavepoint() for exactly this per-row rollback case. Each iteration also compiles a brand-new transaction function via raw.transaction(...), adding per-row overhead on top of the batch transaction. Use the established savepoint helper instead.
💡 Suggested Change
Before:
if (!options.dryRun) db.tx(() => insertRow(kind, row, repos));
After:
if (!options.dryRun) withSavepoint(db, "import_row", () => insertRow(kind, row, repos));
Owner identifiers are taken verbatim from the bundle, without the normalizeNamespace()/cleanId() normalization applied to every other owner write path (lower-casing, whitespace trimming, non-alphanumeric collapsing). A bundle row with ownerProfileId: "Work" or ownerAgentKind: "Claude Code" therefore stores a value that never matches isVisibleTo/ownerWhere, so the row is imported yet permanently invisible to its owner. Also note validationIssue accepts an empty string here; normalize (and treat empty as absent) before persisting.
💡 Suggested Change
Before:
return {
ownerAgentKind: (row.ownerAgentKind as string) ?? fallback.ownerAgentKind,
ownerProfileId: (row.ownerProfileId as string) ?? fallback.ownerProfileId,
The minimum Node version is hardcoded here as 22, although it is already declared in package.json (engines.node: ">=22.0.0") and checked by scripts/verify-native-runtime.cjs. Three independent copies can drift (and this PR already had to bump all three). Prefer deriving it from a single shared constant/metadata source, and guard process.versions.node access so exotic runtimes still surface a MemosError rather than a raw TypeError.
dedupeTraceIds is now unused: its only caller lived in the import code that was moved out of this file (the remaining hit is an unrelated local helper in core/capture/capture.ts). This is dead code that lint may flag as unused; either delete it or re-export it from the new import module if still needed.
The namespace captured here is activeNamespace at call time; the remaining result counters (visible/hidden) are computed against it even when ownership: "target" + targetProfileId re-home rows into a different profile, so a migration-target preview reports every row as hidden. Either count visibility against the resolved target namespace, or document this explicitly — otherwise the hidden count is misleading in the viewer.
34. apps/memos-local-plugin/install.ps1 (L373)
Same issue as in install.sh: --strict-allow-scripts (and the allowScripts field it enforces) only exists in npm 11.17+, and Node 22/24 on Windows ships npm 10.x. There is no npm version check in this script, and --loglevel=error hides npm's "Unknown cli config" warning, so the intended lifecycle-script hardening may silently not apply. Consider verifying npm --version >= 11.17 before this call and failing closed with an actionable message.
This native probe writes its failure text to stderr and then relies solely on $LASTEXITCODE, which contradicts the pattern documented earlier in this script (Invoke-OpenClawGatewayChecked: "PowerShell 5.1 can promote a native process' stderr to an ErrorRecord"). Under pwsh with $PSNativeCommandUseErrorActionPreference = $true (or 5.1 stderr promotion) the console.error output becomes a terminating error before the $LASTEXITCODE check runs, so the user gets a raw ErrorRecord instead of the intended "Bridge Node.js runtime is unsupported" message. Also note the Remove-Item -Recurse -Force $StagedPrefix here runs without -ErrorAction SilentlyContinue under $ErrorActionPreference = "Stop", so a locked file replaces the intended error message. Capture stderr / suppress promotion the same way as the gateway helper and make the cleanup tolerant.
💡 Suggested Change
Before:
& $NodeForBridge -e 'if (Number(process.versions.node.split(".")[0]) < 22) { console.error("MemOS requires Node.js >=22 in the bridge runtime"); process.exit(1); }'
if ($LASTEXITCODE -ne 0) { Remove-Item -Recurse -Force $StagedPrefix; throw "Bridge Node.js runtime is unsupported; existing installation is unchanged." }
The staged prefix lives in mktemp -d and is only removed on the explicit failure branches below and on the success path. The only global cleanup hook is trap cleanup_install_state EXIT (line 326), which removes STAGE_DIR and DSH_PNPM_TEMP_DIR but knows nothing about staged_prefix. So this die (and die "Node.js not found after bootstrap.", or Ctrl-C during the multi-minute npm install) leaves a complete, freshly installed node_modules (hundreds of MB with onnxruntime) in $TMPDIR. Please make staged_prefix a global managed by the existing EXIT trap (e.g. TARBALL_STAGE_PREFIX) instead of a local, so every exit path cleans it up.
💡 Suggested Change
Before:
[[ -d "${staged_prefix}/node_modules" ]] || die "npm install failed in ${staged_prefix}"
After:
[[ -d "${staged_prefix}/node_modules" ]] || die "npm install failed in ${staged_prefix}"
# staged_prefix must be a global cleaned by cleanup_install_state so that
# die/Ctrl-C do not leave a full node_modules tree behind in $TMPDIR.
37. apps/memos-local-plugin/install.sh (L1466)
This inline policy is a third copy of the reviewed build-script policy and it already diverges from the canonical ones: both pnpm-workspace.yaml (line 10) and package.json#allowScripts deny fsevents, but this map omits it. Because strictDepBuilds: true is written right below, a macOS DSH profile that resolves fsevents (optional watcher dependency) will make pnpm report an ignored build and install_dsh will now abort with "DSH reported pending scripts after policy preparation" — a failure with no recovery path since approve-builds was removed. Please mirror fsevents: false here (and ideally derive this map from a single source instead of duplicating it).
--strict-allow-scripts relies on npm's allowScripts/strict-allow-scripts support that the repo itself only ships with npm 11.17.0 (scripts/verify-script-policy.cjs asserts "Run through npm run verify:script-policy with npm 11.17.0" and CI pins npm@11.17.0), but the installer never verifies the npm version. Node 22 ships npm 10.x, where the flag and the allowScripts field are unknown: npm only prints a warning, and that warning is suppressed by --loglevel=error, so the hardening silently no-ops and dependencies' install scripts (e.g. onnxruntime-node's CUDA download, protobufjs) run anyway. Please assert a minimum npm version (or fail closed) before relying on these flags.
39. apps/memos-local-plugin/install.sh (L214)
Now that ensure_node only dies instead of bootstrapping, install_node_mac (line 179) and install_node_linux (line 188) have no callers left, and their die "No supported package manager..." branch is unreachable. Either delete the dead bootstrap helpers plus download_to_file/run_with_privilege (if unused elsewhere) or keep the auto-install path, so the installer's contract is unambiguous for future editors.
40. apps/memos-local-plugin/install.sh (L387)
node_version is assigned but no longer used anywhere in deploy_tarball_to_prefix (it was only consumed by the removed npm rebuild step and its log message). Drop it to avoid shellcheck/SC2034-style noise in the hot install path.
41. apps/memos-local-plugin/install.sh (L414)
The live prefix is destroyed (rm -rf "${prefix}") before this copy, but unlike Deploy-Tarball in install.ps1 there is no rollback/compensation here, and saved_dir (holding data, config.yaml, .auth.json, …) is only restored on the happy path and is not tracked by trap cleanup_install_state EXIT. With set -e, any failure of this now much larger copy (a full node_modules tree across filesystems, ENOSPC, permission error in ~/.openclaw) aborts the script with the user's runtime data stranded in a /tmp mktemp directory and the live prefix empty/partial. Consider staging with mv when the source and target are on the same filesystem, and registering saved_dir/a rollback (as done for Hermes via HERMES_PREFIX_BACKUP_ROOT) so the preserve set is restored on any failure.
💡 Suggested Change
Before:
cp -R "${staged_prefix}/." "${prefix}/"
After:
if ! cp -R "${staged_prefix}/." "${prefix}/"; then
# restore the preserved user-data artefacts before aborting
for item in "${preserve[@]}"; do
if [[ -e "${saved_dir}/${item}" ]]; then
rm -rf "${prefix:?}/${item}"
mv "${saved_dir}/${item}" "${prefix}/${item}" || true
fi
done
rm -rf "${saved_dir}"
die "Deploy failed; preserved user data was restored and the installation is incomplete."
fi
The backup path is reserved with openSync(..., "wx") before db.backup(), but nothing removes that placeholder if db.backup() or the subsequent transaction throws (e.g. a skill-name conflict). After such a failure a zero-byte file remains, so every retry rejects with EEXIST and an operator may mistake the empty placeholder for a valid backup. Also note the dry run cannot surface merge conflicts, so collisions are only discovered at --apply time. Consider removing the reserved file on failure and reporting potential conflicts (e.g. skills name duplicates) in the dry-run output.
The repair reports success without ever checking whether the source profile actually had rows (from/to may be swapped, or the operator may point at the wrong DB). In that case the transaction moves nothing, a backup file is still created, and the output looks like a successful migration. Consider refusing (or at least warning) when every count is 0, and re-reading the counts after the transaction to verify no rows remain for from, e.g. const remaining = ... WHERE owner_agent_kind=? AND owner_profile_id=? ... and throwing if non-zero.
These rules are re-implemented here and again in importMemoryBundle (which already throws MemosError("invalid_argument", ...) for the same conditions, including the ownership/targetProfileId pairing and dryRun type). The copies will drift (e.g. only lowercase true/false is accepted here, so dryRun=True becomes a generic 400 instead of the importer's specific message). Consider delegating validation to the core and mapping its MemosError to a 400.
Two problems here beyond duplicated validation: (1) ownership=dryRun-style typos in the query string are silently accepted only for exact lowercase values, while dryRun=TRUE/targetProfileId= with whitespace produce opaque 400s; (2) more importantly, an unknown/misspelled query param is ignored entirely, so ?ownership=target&targetProfile=work (missing Id) will import into the active profile instead of the named one — a destructive no-op the caller cannot detect. Validate against the returned MemoryImportResult.targetProfileId or reject unknown query keys.
Adding local as the third parameter (after the injectable create factory) forces every non-injected call site to spell out undefined, which is easy to mis-place and hides the real argument. Consider putting the new parameter second so the production call becomes probeLocalEmbedding(model, req.local).
local now comes straight from the untrusted request body (parseJson<TestRequest>) and is only typed, never validated or coerced. Downstream code does config.local?.loadTimeoutMs ?? 120_000 (a string value yields NaN → setTimeout fires immediately and aborts the probe) and resolves modelPath/cacheDir via path.resolve (arbitrary filesystem paths from an HTTP caller). Validate/coerce the block with a small TypeBox guard (or reuse EmbeddingConfig.local schema) before passing it to the embedder, and reject non-localhost callers when the viewer is bound to a non-loopback host.
local is also surfaced to probeLocalEmbedding per request, but the probe never hydrates this block from the saved embedding.local (unlike apiKey/endpoint, which resolveSecrets restores). If the client omits or partially sends local, the test silently runs against defaults (~/.cache/memos/models, offline:false) instead of the persisted settings, so "test" can pass/fail differently from the real runtime. Either hydrate local from the saved config when the caller sends none, or document the divergence next to the endpoint.
Now that the probe honours embedding.local, createLocalProbeEmbedder can fail for reasons unrelated to Hugging Face reachability: localSettings() in core/embedding/providers/local.ts throws MemosError("config_invalid", "Local model remoteHost/HF_ENDPOINT must be an HTTP(S) base URL ..."), and offline: true fails with a cache-miss error. The catch below rewrites every one of these into "Local embedding model download/load failed; check access to Hugging Face: ", so a bad mirror URL / offline misconfiguration is reported as a Hugging Face connectivity problem. Consider preserving MemosError codes/messages (rethrow when MemosError.is(err) or when the cause is a config error) so the Settings test button shows the actionable message.
Every event produced by this helper carries seq: 1 / time: 1, so a test that encodes several durable events (e.g. agent/inbox/spliced plus user/message in the bridge test) ends up with duplicate sequence numbers that the real DSH event stream never produces. That can mask ordering/deduplication bugs in the code under test. Consider accepting optional seq/time overrides (defaulting to 1) so callers can emit a monotonically increasing sequence.
kind collapses the whole operation to "error" as soon as any row is invalid/failed, even when imported > 0. A bundle with 500 valid + 1 invalid row will render in red, and since the message only changes color (no severity wording), users will assume nothing was imported and may re-upload. Consider deriving the severity from imported: "ok" when imported > 0, "error" only when nothing was imported; keep the invalid/failed counts in the message.
The diagnostics list prints internal identifiers verbatim (trace[3], write_failed, raw field names) to end users; these are contract enums from agent-contract/import.ts and are not localized, which is inconsistent with the i18n work added in this PR. Consider mapping kind/code to translated labels (with the raw code kept only in a title/tooltip), and adding the <ul> spacing via a class instead of the unstyled list.
The file input value is never cleared, so re-selecting the same file after an import/error will not fire onChange again (browsers suppress change when the value is unchanged). Reset the input so the identical bundle can be re-previewed/re-imported.
💡 Suggested Change
Before:
if (f) { setFile(f); resetPreview(); void run(f, true); }
After:
if (f) { setFile(f); resetPreview(); void run(f, true); }
(e.target as HTMLInputElement).value = "";
MemoryImportResult.issues is capped at 50 entries server-side (bundle-import.ts), but the list is rendered as if it were complete. A bundle with hundreds of rejected rows will only surface the first 50 diagnostics with no indication that the rest were dropped. Consider showing a truncation notice (e.g. when issues.length === 50) so users know to re-check the remaining rows.
The local load-timeout default (120_000) and the valid range (1–3600 s) are duplicated here, while the canonical values live in core/config/defaults.ts and core/config/schema.ts (loadTimeoutMs: NumberInRange(120_000, 1, 3_600_000)). If core changes the default/bounds the UI silently drifts. Also Number(value) || 120 makes an empty input snap back to 120 s, so the field can never be cleared while typing. Suggest importing the shared default bounds constant and using ?? default on the parsed value instead of ||.
These two include patterns are appended after the ! exclusion block, which makes files order-dependent and hard to reason about (any future negative pattern added below them silently changes behavior). They belong with the other scripts/* includes at the top. Also note scripts/verify-script-policy.cjs is referenced by release:validate (npm run verify:script-policy) but is not shipped, unlike its siblings normalize-sqlite-lock.cjs/verify-native-runtime.cjs — confirm whether it is intentionally excluded (dev/CI-only).
A failing child order is undiagnosable: promisified execFile rejects, and the child's stderr (where Node prints the AssertionError with the offending added DLL list or worker pid) is only attached to err.stderr — it is never written out. The parent then just reports Command failed: node ... --order plugin-first, which defeats the purpose of this guard on CI. Catch and re-emit the child output before rethrowing.
process.exit(0) right after process.stdout.write(...) can truncate the piped child output; when stdout is a pipe (CI log capture / execFile), pending writes are not guaranteed to flush. The JSON verdict plus assertion details are the only output of this check, so drop the explicit exit and let the event loop drain (or use process.exitCode = 0).
This negative control can pass for the wrong reason. Any non-zero npm ci exit satisfies assert.notEqual(install.status, 0), and the package name appears in unrelated npm errors too (e.g. a lockfile/tarball mismatch prints Invalid: lock file's memos-unreviewed-policy-probe@1.0.0 does not satisfy ...), so a policy that never rejects scripts would still look green. Spawn-level failures are also not distinguished: spawnSync returns status: null with error set on timeout (30 s is tight for a cold npm ci on Windows CI) or when npm_execpath is not a JS entry, and notEqual(null, 0) passes. Assert the failure is policy-specific and reject spawn errors explicitly.
assert.equal(install.error, undefined, `npm ci could not be spawned: ${install.error}`);
assert.equal(install.signal, null, "npm ci timed out instead of failing on the script policy");
assert.notEqual(install.status, 0, "Unreviewed install script did not fail installation");
assert.match(install.stdout + install.stderr, /memos-unreviewed-policy-probe/);
assert.match(
install.stdout + install.stderr,
/allowScripts|strict-allow-scripts|not allowed|blocked/i,
"npm ci failed for a reason unrelated to the script policy",
);
require("better-sqlite3/package.json") goes through the package's exports map; if upstream ever adds one without a ./package.json entry, this throws ERR_PACKAGE_PATH_NOT_EXPORTEDafter every real assertion succeeded, failing the probe for a purely cosmetic reason. Resolve it as a relative path from the package entry instead (the file already imports createRequire).
engines.node now admits every Node >=22, including 22.0–22.18 and all of the 23.x line, while the actual runtime gate is stricter: install.sh refuses anything outside ^22.19.0 || >=24.0.0 (install.sh:1531-1532) and adapters/deepseek-harness/README.md:119 documents the same stricter host requirement. With engine-strict=true in .npmrc, npm reports success on a host the DSH install path (and the Node-API prebuild smoke check) will then reject, so the failure surfaces far from the cause. Suggest aligning the declaration with the enforced range.
This policy relies entirely on pnpm 11 semantics (strictDepBuilds / allowBuilds, and the "esbuild@0.27.7" version-qualified keys), but nothing pins that toolchain version — there is no packageManager/devEngines field in package.json. Running pnpm install here with an older pnpm silently ignores these keys (the file is still a valid config file), so no unapproved build script is blocked and the security intent of the policy is lost without any error. Consider pinning the package manager version so the policy cannot be bypassed by invoking the wrong pnpm.
💡 Suggested Change
Before:
strictDepBuilds: true
After:
strictDepBuilds: true
# Requires pnpm >=11 (allowBuilds/strictDepBuilds); pin the version, e.g.
# "packageManager": "pnpm@11.7.0" in package.json
This assertion assumes process.report.getReport().sharedObjects is always an array. Node only populates sharedObjects on Linux/AIX; on Windows (the only platform this script runs on, and it is documented to be executed under the DSH Electron binary via ELECTRON_RUN_AS_NODE) the field is absent, so .filter(...) throws TypeError: Cannot read properties of undefined right after the vector assertions pass — the probe crashes instead of reporting a DLL leak. The earlier new Set(process.report.getReport().sharedObjects) silently degrades to an empty set, which hides the same problem, and if the list were present but empty the comparison would be vacuously true. Capture the list defensively and fail loudly when the runtime does not expose it, so the check can never pass/report for the wrong reason.
const after = process.report.getReport().sharedObjects ?? [];
assert.ok(
after.length > 0,
"process.report did not expose sharedObjects on this runtime; DLL isolation cannot be verified",
);
const added = after.filter(
🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (duplicate: 1).
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
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.