Harden desktop runtime and add multilingual memory - #169
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change replaces MiniLM with pinned multilingual E5-small embeddings, adds query/passage handling and migrations, narrows Electron IPC access, protects settings and filesystem boundaries, updates VAD packaging, and adjusts startup and cancellation behavior. ChangesMultilingual embedding migration
Electron security boundaries
Runtime and lifecycle updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant aliceIPC
participant MainProcess
participant Backend
participant VectorStore
Renderer->>aliceIPC: Request embedding with input_type
aliceIPC->>MainProcess: Validate IPC channel
MainProcess->>Backend: Send prefixed query or passage text
Backend->>VectorStore: Return embedding vector
VectorStore->>MainProcess: Store or reindex current-model vector
MainProcess->>aliceIPC: Return result
aliceIPC->>Renderer: Deliver response payload
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/utils/functions/filesystem.ts (1)
48-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the
desktopAPIbridge asopen_pathdoes.
open_pathcheckswindow.aliceIPC?.invokeand returns a clear error.list_directoryat Line 52 andexecute_commandat Line 67 callwindow.desktopAPIwithout a guard. If the bridge is absent, both throw aTypeError, and the catch block returns that opaque message to the model instead of a clear reason.🛡️ Proposed guard for both functions
+function requireDesktopAPI(): NonNullable<typeof window.desktopAPI> { + if (typeof window === 'undefined' || !window.desktopAPI) { + throw new Error( + 'Desktop bridge not available. This function only works in the desktop app.' + ) + } + return window.desktopAPI +} + export async function list_directory( args: ListDirectoryArgs ): Promise<FunctionResult> { try { - const result = await window.desktopAPI.listDirectory(args.path) + const result = await requireDesktopAPI().listDirectory(args.path)Apply the same call in
execute_command:const result = await requireDesktopAPI().executeCommand(args.command)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/functions/filesystem.ts` around lines 48 - 61, Guard the desktop bridge in both list_directory and execute_command by reusing requireDesktopAPI(), matching the existing open_path behavior. Replace direct window.desktopAPI calls with the required bridge access so missing APIs return the established clear error instead of an opaque TypeError, while preserving the existing success and failure result handling.src/composables/useAudioProcessing.ts (1)
330-340: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the global listener registration state during cleanup.
After Line 331 removes the listeners,
ipcListenersRegisteredremainstrue. If this composable mounts again, Line 85 skips registration and global hotkeys stop working.Proposed fix
onUnmounted(() => { destroyVAD() if (window.aliceIPC) { window.aliceIPC.off('global-hotkey-mic-toggle', handleGlobalMicToggle) window.aliceIPC.off( 'global-hotkey-mute-playback', handleGlobalMutePlayback ) window.aliceIPC.off( 'global-hotkey-take-screenshot', handleGlobalTakeScreenshot ) } + ipcListenersRegistered = false })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/composables/useAudioProcessing.ts` around lines 330 - 340, Update the cleanup block in useAudioProcessing after removing the global hotkey listeners to reset ipcListenersRegistered to false. Ensure subsequent composable mounts can pass the registration guard and re-register all global listeners.src/components/Chat.vue (1)
248-262: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove the
window.openfallback and validate external URLs in the main process.electron:open-pathsends everyhttp://,https://, andmailto:target directly toshell.openExternalwithout URL validation. BothsetWindowOpenHandlerhandlers deny popups but callshell.openExternalfor every HTTP(S) URL before returningdeny. No navigation handler enforces the policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Chat.vue` around lines 248 - 262, Remove both window.open(href, '_blank', 'noopener,noreferrer') fallbacks from the electron:open-path success and error paths in Chat.vue, and rely solely on the IPC result. In the main-process electron:open-path handler and both setWindowOpenHandler handlers, validate targets against the approved external URL schemes and destinations before calling shell.openExternal; reject invalid URLs and preserve popup denial without opening them.backend/internal/minilm/onnx_embeddings.go (1)
329-343: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate tokenizer failures.
Line 329 discards
EncodeSingleerrors. The method then returns zero IDs and a zero attention mask.GenerateEmbeddingsreturns a successful zero vector for that text.Return the tokenizer error through
encodeandbatchTokenize. Stop the embedding request when tokenization fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/minilm/onnx_embeddings.go` around lines 329 - 343, The tokenizer error from EncodeSingle in encode is currently discarded; propagate it through encode and batchTokenize, updating their signatures and callers as needed. Ensure GenerateEmbeddings stops and returns the error when tokenization fails instead of producing a zero-vector embedding, while preserving normal padding and truncation behavior.
🧹 Nitpick comments (8)
electron/main/settingsManager.ts (1)
261-268: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite the protected secrets file atomically.
writePrivateFiletruncates the target and then writes.alice-secrets.binis the only copy of the credentials after migration removes the plaintext values. If the process stops between truncation and completion, the file is empty or partial,loadEncryptedSecretsthrows, and the user must re-enter every credential.Write to a temporary file in the same directory and then rename it.
fs.renameis atomic within one filesystem.♻️ Proposed atomic write
async function writePrivateFile( filePath: string, data: string | Buffer, encoding?: BufferEncoding ): Promise<void> { - await fs.writeFile(filePath, data, { encoding, mode: 0o600 }) - await fs.chmod(filePath, 0o600) + const tempPath = `${filePath}.${process.pid}.tmp` + try { + await fs.writeFile(tempPath, data, { encoding, mode: 0o600 }) + await fs.chmod(tempPath, 0o600) + await fs.rename(tempPath, filePath) + } catch (error) { + await fs.rm(tempPath, { force: true }) + throw error + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main/settingsManager.ts` around lines 261 - 268, Update writePrivateFile to write the data to a uniquely named temporary file in the target file’s directory with mode 0o600, then atomically replace the target using fs.rename; ensure temporary-file cleanup on failure while preserving the existing encoding and permissions.src/utils/functions/filesystem.ts (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the obsolete renderer approval state
The main process now prompts for every command and supports only “Run once”. Remove
approvedCommands, the unused approval helpers, and the Security settings UI, which incorrectly claims that commands can run without approval.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/functions/filesystem.ts` around lines 63 - 67, Remove the obsolete approvedCommands state and its associated approval helper functions, then remove the Security settings UI and any references to it. Update command execution around execute_command to rely solely on the main process prompt and “Run once” behavior, with no renderer-side approval or persistent command authorization.electron/preload/index.ts (1)
77-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing only tracked wrappers in
removeAllListeners.
ipcRenderer.removeAllListeners(channel)detaches every listener on the channel. If two renderer modules subscribe to the same event, such asoverlay-shown, then one module's cleanup removes the other module's handler. Removing only the wrappers tracked inlistenerWrapperskeeps modules isolated.♻️ Proposed change
removeAllListeners(channel: string) { assertAllowedChannel(channel, isAllowedEventChannel, 'event') - ipcRenderer.removeAllListeners(channel) - listenerWrappers.delete(channel) + const channelListeners = listenerWrappers.get(channel) + if (channelListeners) { + for (const wrappers of channelListeners.values()) { + for (const wrapper of wrappers) { + ipcRenderer.off(channel, wrapper) + } + } + listenerWrappers.delete(channel) + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/preload/index.ts` around lines 77 - 81, Update removeAllListeners to remove only the listener wrappers tracked for the specified channel instead of calling ipcRenderer.removeAllListeners(channel), then clear that channel’s tracked wrappers from listenerWrappers. Preserve channel validation and ensure unrelated modules’ listeners remain attached.src/composables/useScreenshot.ts (1)
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
aliceIPCaccess pattern consistent.Line 14 derives
isElectronfromwindow.electron, not fromwindow.aliceIPC. Line 68 uses optional chaining, but lines 72 and 76 do not. If the preload exposeselectronwithoutaliceIPC, then line 72 throws and listener setup aborts silently. Gate the block onwindow.aliceIPCinstead.♻️ Proposed change
const setupScreenshotListeners = () => { - if (isElectron) { + if (isElectron && window.aliceIPC) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/composables/useScreenshot.ts` around lines 68 - 76, Update the listener-registration block around handleScreenshotCapturedListener and handleOverlayClosedListener to first verify that window.aliceIPC exists, rather than relying on the isElectron check derived from window.electron. Keep both aliceIPC.on registrations inside that guard so missing aliceIPC skips setup without throwing.electron/main/ragDocumentStore.ts (1)
105-105: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the
rag_chunks.embedding_modeldefault in the CREATE TABLE statements.
rag_documentsusesRAG_EMBEDDING_MODELas the column default.rag_chunksusesLEGACY_RAG_EMBEDDING_MODEL. InCREATE TABLE IF NOT EXISTSthe default applies to a fresh database, where no legacy chunk can exist. All current insert paths passembedding_modelexplicitly, so behavior is correct today. If a later insert omits the column, new chunks are marked legacy,countLegacyEmbeddingskeeps returning a nonzero count, andreindexRagIfNeededreindexes on every launch.Use the legacy default only in
ensureRagEmbeddingModelColumns, where it describes pre-existing rows.♻️ Proposed change
- embedding_model TEXT NOT NULL DEFAULT '${LEGACY_RAG_EMBEDDING_MODEL}', + embedding_model TEXT NOT NULL DEFAULT '${RAG_EMBEDDING_MODEL}',Apply this to both
rag_chunksdefinitions (Line 118 and Line 156). Keep the legacy default inensureRagEmbeddingModelColumns.Also applies to: 118-118, 144-144, 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main/ragDocumentStore.ts` at line 105, Update both rag_chunks CREATE TABLE definitions in the relevant initialization paths to use RAG_EMBEDDING_MODEL as the embedding_model default, matching rag_documents. Keep LEGACY_RAG_EMBEDDING_MODEL only in ensureRagEmbeddingModelColumns for existing rows and migration handling.src/services/backendApi.ts (1)
367-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the divergent default input types.
generateEmbeddingdefaults toqueryandgenerateEmbeddingsdefaults topassage. The asymmetry is easy to miss. A caller that batches user queries receives passage prefixes and gets degraded retrieval scores, with no error. Add a short doc note on each method, or require the parameter explicitly.♻️ Proposed doc clarification
/** - * Generate embedding for text + * Generate embedding for text. + * Defaults to the `query` input type, because single-text calls are + * retrieval queries. Pass `passage` when embedding stored content. */ async generateEmbedding(/** - * Generate embeddings for multiple texts + * Generate embeddings for multiple texts. + * Defaults to the `passage` input type, because batch calls are indexing + * runs. Pass `query` when embedding a batch of retrieval queries. */ async generateEmbeddings(Also applies to: 391-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/backendApi.ts` around lines 367 - 375, Document the differing defaults in generateEmbedding and generateEmbeddings: identify that the single-text method defaults inputType to query while the batch method defaults to passage, and state when callers should choose each type. Keep the current defaults and behavior unchanged unless making inputType required is necessary.electron/main/index.ts (1)
7-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
app.disableHardwareAcceleration()below all imports.ES module imports are hoisted. The call at Line 9 runs after every import in this file is evaluated, including
./thoughtVectorStoreand./ragDocumentStoreat Lines 11-16. The placement suggests it runs before those module side effects, and it does not.The call still happens before the
readyevent, so behavior is correct. Moving it below the import block removes the misleading ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main/index.ts` around lines 7 - 16, Move the app.disableHardwareAcceleration() call below the complete import block in electron/main/index.ts, including the thoughtVectorStore and ragDocumentStore imports, while keeping it before application initialization or ready-event handling.electron/main/thoughtVectorStore.ts (1)
302-315: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider bounding the pending-row selection.
Both
.all()calls materialize every pending row, including the fulltext_contentandcontentvalues, in the main process before any batching starts. For a large history this is one large allocation on top of the per-batch slices.Selecting one batch at a time keeps memory bounded and preserves the existing idempotency, because each update already filters on
embedding_local IS NULL.const selectBatch = db.prepare( 'SELECT thought_id as id, text_content as text FROM thoughts WHERE embedding_local IS NULL LIMIT ?' ) // loop: read a batch, embed it, update it, repeat until the batch is empty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main/thoughtVectorStore.ts` around lines 302 - 315, Update the pending-row processing around the thoughts and memories queries to fetch bounded batches instead of materializing all rows with unbounded .all() calls. Reuse prepared batch queries with a LIMIT, repeatedly embed and update each batch, and stop when a batch is empty; preserve the existing embedding_local IS NULL filters and waitForLocalEmbeddingsReady behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/api/embeddings.go`:
- Around line 29-38: Update prefixEmbeddingText to distinguish an omitted
inputType from an explicitly provided value: preserve passage as the default
when omitted, accept only query or passage, and reject any other explicit value
such as "qurey" instead of silently applying the passage prefix. Propagate the
validation error through the embedding request flow.
In `@backend/internal/minilm/tokenizer_test.go`:
- Around line 12-15: Update the Go backend workflow before the go test step to
run npm run setup:embeddings, ensuring the multilingual tokenizer artifact
required by the tokenizer tests is installed and the test is not skipped.
In `@electron/main/desktopManager.ts`:
- Around line 51-76: Update the approval dialog in the command execution handler
to display only a bounded excerpt of command, rather than the full accepted
input. When truncation occurs, append a clear indication that the command is
truncated, while preserving the existing confirmation flow and using the bounded
text for both dialog variants.
- Around line 78-90: Update the exec call in the surrounding command-execution
method to configure both a finite timeout and an appropriately increased
maxBuffer. In its callback, use only the error argument to determine success;
when no error exists, resolve success while preserving stdout and including
stderr as additional context instead of treating it as failure. Keep genuine
execution errors on the failure path.
In `@electron/main/ipcManager.ts`:
- Around line 1225-1229: Cache the result of getAllowedHttpOrigins(settings) in
the main-process HTTP bridge instead of calling loadSettings() for every request
and stream start. Reuse the cached allowed-origin set in the handlers around
validateHttpBridgeUrl, and invalidate or refresh that cache in the existing
settings:save handler so saved settings take effect immediately.
In `@electron/main/memoryManager.ts`:
- Around line 126-136: Replace the ASCII-only LIKE matching in the memory search
flow with the existing FTS5/unicode61 approach used by ragDocumentStore.ts,
querying long_term_memories with prefix terms so tokenized non-ASCII queries
match stored content and avoid leading-wildcard scans. Preserve the memoryType
filter, created_at ordering, and limit behavior.
In `@electron/main/ragDocumentStore.ts`:
- Around line 1331-1346: Update countLegacyEmbeddings() to count only legacy
documents whose paths are included in the configured normalizedTargets used by
pruneMissingDocuments(), or explicitly exclude and resolve out-of-scope legacy
rows. Ensure reindexRagIfNeeded() does not repeatedly retry legacy documents
outside the configured paths.
In `@electron/main/settingsManager.ts`:
- Around line 101-121: Update saveSettings around splitSecretSettings and the
empty-secrets branch to use the returned hadSecretFields flag: only clear the
encrypted secrets file when secret fields were submitted and resolved to empty
values, while preserving stored credentials for partial settings without secret
keys. Add a test in settingsSecurity.test.ts asserting hadSecretFields is false
when no secret key is present.
In `@electron/main/thoughtVectorStore.ts`:
- Around line 185-197: In the migration flow containing
LOCAL_EMBEDDING_INVALIDATION_FLAG and hnswLocalIndexFilePath, move the database
flag update until after the stale HNSW index removal completes successfully.
Preserve the outer failure handling so unlinkSync errors propagate to retry the
migration on the next launch; do not mark the migration flag before deletion
succeeds.
In `@electron/preload/index.ts`:
- Around line 152-153: Update the executeCommand API and its
desktop:executeCommand handler to accept only structured commands from an
explicit allowlist rather than passing arbitrary strings to child_process.exec.
Validate the command name and permitted arguments before execution, reject
unknown or malformed requests, and preserve user confirmation for allowed
commands.
In `@electron/preload/ipcBridgePolicy.ts`:
- Around line 3-81: Unify the desktop IPC bridge with the central allowlist: in
electron/preload/ipcBridgePolicy.ts lines 3-81, add desktop:listDirectory and
desktop:executeCommand to INVOKE_CHANNELS; in electron/preload/index.ts lines
149-155, update both desktopAPI methods to call aliceIPC.invoke so
assertAllowedChannel is applied consistently. Confirm renderer filesystem
helpers use this bridge and preserve their existing behavior.
In `@package.json`:
- Around line 63-84: Add onnxruntime-web as a pinned direct dependency in the
package.json dependencies list, then regenerate package-lock.json so the direct
dependency and its resolved package metadata are recorded. Keep the existing
build/vadAssets.ts and vite.config.ts resolution behavior unchanged.
In `@scripts/setup-embeddings.js`:
- Around line 147-166: Update the platform setup try/catch around downloadFile,
extractArchive, and the copy logic to validate each archive or extracted library
against a configured expected SHA-256 before copying. Treat missing files,
checksum mismatches, download, extraction, and copy errors as failures by
rethrowing from the catch instead of only logging, so the overall setup cannot
report success after a platform error.
- Around line 239-241: Update the main-module check around main() to compare
import.meta.url with pathToFileURL(process.argv[1]).href, and only perform the
comparison when process.argv[1] is defined. Import or reuse the pathToFileURL
utility as needed while preserving the existing behavior of invoking main() for
the entrypoint.
In `@src/services/llmProviders/mainProcessStream.ts`:
- Around line 70-87: The stream cleanup around the abort listener must remove
the registered abort handler when the stream finishes, not only rely on `{ once:
true }`. Update the cleanup/finally path for the listener and `abort` symbols to
call removeEventListener on the same signal, preserving cancellation behavior
while preventing stale http:stream-cancel requests.
---
Outside diff comments:
In `@backend/internal/minilm/onnx_embeddings.go`:
- Around line 329-343: The tokenizer error from EncodeSingle in encode is
currently discarded; propagate it through encode and batchTokenize, updating
their signatures and callers as needed. Ensure GenerateEmbeddings stops and
returns the error when tokenization fails instead of producing a zero-vector
embedding, while preserving normal padding and truncation behavior.
In `@src/components/Chat.vue`:
- Around line 248-262: Remove both window.open(href, '_blank',
'noopener,noreferrer') fallbacks from the electron:open-path success and error
paths in Chat.vue, and rely solely on the IPC result. In the main-process
electron:open-path handler and both setWindowOpenHandler handlers, validate
targets against the approved external URL schemes and destinations before
calling shell.openExternal; reject invalid URLs and preserve popup denial
without opening them.
In `@src/composables/useAudioProcessing.ts`:
- Around line 330-340: Update the cleanup block in useAudioProcessing after
removing the global hotkey listeners to reset ipcListenersRegistered to false.
Ensure subsequent composable mounts can pass the registration guard and
re-register all global listeners.
In `@src/utils/functions/filesystem.ts`:
- Around line 48-61: Guard the desktop bridge in both list_directory and
execute_command by reusing requireDesktopAPI(), matching the existing open_path
behavior. Replace direct window.desktopAPI calls with the required bridge access
so missing APIs return the established clear error instead of an opaque
TypeError, while preserving the existing success and failure result handling.
---
Nitpick comments:
In `@electron/main/index.ts`:
- Around line 7-16: Move the app.disableHardwareAcceleration() call below the
complete import block in electron/main/index.ts, including the
thoughtVectorStore and ragDocumentStore imports, while keeping it before
application initialization or ready-event handling.
In `@electron/main/ragDocumentStore.ts`:
- Line 105: Update both rag_chunks CREATE TABLE definitions in the relevant
initialization paths to use RAG_EMBEDDING_MODEL as the embedding_model default,
matching rag_documents. Keep LEGACY_RAG_EMBEDDING_MODEL only in
ensureRagEmbeddingModelColumns for existing rows and migration handling.
In `@electron/main/settingsManager.ts`:
- Around line 261-268: Update writePrivateFile to write the data to a uniquely
named temporary file in the target file’s directory with mode 0o600, then
atomically replace the target using fs.rename; ensure temporary-file cleanup on
failure while preserving the existing encoding and permissions.
In `@electron/main/thoughtVectorStore.ts`:
- Around line 302-315: Update the pending-row processing around the thoughts and
memories queries to fetch bounded batches instead of materializing all rows with
unbounded .all() calls. Reuse prepared batch queries with a LIMIT, repeatedly
embed and update each batch, and stop when a batch is empty; preserve the
existing embedding_local IS NULL filters and waitForLocalEmbeddingsReady
behavior.
In `@electron/preload/index.ts`:
- Around line 77-81: Update removeAllListeners to remove only the listener
wrappers tracked for the specified channel instead of calling
ipcRenderer.removeAllListeners(channel), then clear that channel’s tracked
wrappers from listenerWrappers. Preserve channel validation and ensure unrelated
modules’ listeners remain attached.
In `@src/composables/useScreenshot.ts`:
- Around line 68-76: Update the listener-registration block around
handleScreenshotCapturedListener and handleOverlayClosedListener to first verify
that window.aliceIPC exists, rather than relying on the isElectron check derived
from window.electron. Keep both aliceIPC.on registrations inside that guard so
missing aliceIPC skips setup without throwing.
In `@src/services/backendApi.ts`:
- Around line 367-375: Document the differing defaults in generateEmbedding and
generateEmbeddings: identify that the single-text method defaults inputType to
query while the batch method defaults to passage, and state when callers should
choose each type. Keep the current defaults and behavior unchanged unless making
inputType required is necessary.
In `@src/utils/functions/filesystem.ts`:
- Around line 63-67: Remove the obsolete approvedCommands state and its
associated approval helper functions, then remove the Security settings UI and
any references to it. Update command execution around execute_command to rely
solely on the main process prompt and “Run once” behavior, with no renderer-side
approval or persistent command authorization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd1f9c5a-27bd-4552-8f76-c622b46bc2d8
⛔ Files ignored due to path filters (2)
backend/go.sumis excluded by!**/*.sumpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (66)
README.mdbackend/go.modbackend/internal/api/embeddings.gobackend/internal/api/embeddings_test.gobackend/internal/minilm/onnx_embeddings.gobackend/internal/minilm/tokenizer_test.gobackend/internal/minilm/types.gobackend/internal/models/manager.gobackend/internal/server/server.gobackend/internal/server/server_test.gobackend/main.gobuild/vadAssets.tselectron/main/desktopManager.tselectron/main/index.tselectron/main/ipcManager.tselectron/main/memoryManager.tselectron/main/ragDocumentStore.tselectron/main/securityBoundaries.tselectron/main/settingsManager.tselectron/main/settingsSecurity.tselectron/main/thoughtVectorStore.tselectron/main/updaterManager.tselectron/main/windowManager.tselectron/preload/index.tselectron/preload/ipcBridgePolicy.tspackage.jsonscripts/setup-embeddings.jssrc/App.vuesrc/__tests__/ipcBridgePolicy.test.tssrc/__tests__/securityBoundaries.test.tssrc/__tests__/settingsSecurity.test.tssrc/__tests__/vadRuntime.test.tssrc/components/Actions.vuesrc/components/Chat.vuesrc/components/Main.vuesrc/components/MemoryManager.vuesrc/components/Overlay.vuesrc/components/Settings.vuesrc/components/SettingsWindow.vuesrc/components/Sidebar.vuesrc/components/settings/CoreSettingsTab.vuesrc/components/wizard/OnboardingWizard.vuesrc/composables/useAudioProcessing.tssrc/composables/useCodexAuth.tssrc/composables/useGoogleAuth.tssrc/composables/useScreenshot.tssrc/composables/vadRuntime.tssrc/main.tssrc/modules/conversation/__tests__/speechQueue.test.tssrc/modules/conversation/speechQueue.tssrc/router/main.tssrc/services/apiService.tssrc/services/backendApi.tssrc/services/llmProviders/__tests__/codex.test.tssrc/services/llmProviders/__tests__/openAICompatible.test.tssrc/services/llmProviders/codex.tssrc/services/llmProviders/codexToolBridge.tssrc/services/llmProviders/mainProcessStream.tssrc/stores/conversationStore.tssrc/stores/customToolsStore.tssrc/utils/functionCaller.tssrc/utils/functions/calendar.tssrc/utils/functions/clipboard.tssrc/utils/functions/filesystem.tssrc/vite-env.d.tsvite.config.ts
| return new Promise(resolve => { | ||
| exec(command, (error, stdout, stderr) => { | ||
| if (error) { | ||
| resolve({ success: false, error: error.message }) | ||
| return | ||
| } | ||
| if (stderr) { | ||
| resolve({ success: false, error: stderr }) | ||
| return | ||
| } | ||
| resolve({ success: true, output: stdout }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the command execution and stop treating stderr as failure.
Two problems exist in this block.
- A command that exits with code 0 and writes to
stderris reported as a failure, and itsstdoutis discarded. Many tools write progress and warnings tostderr. Use theerrorargument to decide success, and returnstderras additional context. execruns with notimeoutand the defaultmaxBuffer. A command that never exits leaves the promise pending forever. A command with large output rejects withENOBUFSafter the work is already done. Set both options.
🐛 Proposed fix for classification and bounds
return new Promise(resolve => {
- exec(command, (error, stdout, stderr) => {
- if (error) {
- resolve({ success: false, error: error.message })
- return
- }
- if (stderr) {
- resolve({ success: false, error: stderr })
- return
- }
- resolve({ success: true, output: stdout })
- })
+ exec(
+ command,
+ { timeout: 60_000, maxBuffer: 10 * 1024 * 1024 },
+ (error, stdout, stderr) => {
+ if (error) {
+ resolve({ success: false, error: error.message, output: stdout })
+ return
+ }
+ resolve({ success: true, output: stdout, stderr })
+ }
+ )
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new Promise(resolve => { | |
| exec(command, (error, stdout, stderr) => { | |
| if (error) { | |
| resolve({ success: false, error: error.message }) | |
| return | |
| } | |
| if (stderr) { | |
| resolve({ success: false, error: stderr }) | |
| return | |
| } | |
| resolve({ success: true, output: stdout }) | |
| }) | |
| }) | |
| return new Promise(resolve => { | |
| exec( | |
| command, | |
| { timeout: 60_000, maxBuffer: 10 * 1024 * 1024 }, | |
| (error, stdout, stderr) => { | |
| if (error) { | |
| resolve({ success: false, error: error.message, output: stdout }) | |
| return | |
| } | |
| resolve({ success: true, output: stdout, stderr }) | |
| } | |
| ) | |
| }) |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 79-89: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/main/desktopManager.ts` around lines 78 - 90, Update the exec call
in the surrounding command-execution method to configure both a finite timeout
and an appropriately increased maxBuffer. In its callback, use only the error
argument to determine success; when no error exists, resolve success while
preserving stdout and including stderr as additional context instead of treating
it as failure. Keep genuine execution errors on the failure path.
Source: Linters/SAST tools
| const listener = (event: StreamQueueEvent) => { | ||
| pushEvent(event) | ||
| } | ||
|
|
||
| const abort = () => { | ||
| void window.ipcRenderer.invoke('http:stream-cancel', { requestId }) | ||
| void window.aliceIPC.invoke('http:stream-cancel', { requestId }) | ||
| pushEvent({ type: 'error', error: 'The operation was aborted.' }) | ||
| } | ||
|
|
||
| window.ipcRenderer.on(channel, listener as any) | ||
| window.aliceIPC.on(channel, listener as any) | ||
| signal?.addEventListener('abort', abort, { once: true }) | ||
|
|
||
| try { | ||
| if (signal?.aborted) { | ||
| throw createAbortError() | ||
| } | ||
|
|
||
| const startResult = await window.ipcRenderer.invoke('http:stream-start', { | ||
| const startResult = await window.aliceIPC.invoke('http:stream-start', { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the abort listener during stream cleanup.
If the stream completes before abort, { once: true } does not remove the listener. A later signal abort retains this request closure and sends a stale http:stream-cancel request.
Proposed fix
} finally {
+ signal?.removeEventListener('abort', abort)
window.aliceIPC.off(channel, listener as any)
void window.aliceIPC.invoke('http:stream-cancel', { requestId })
}Also applies to: 122-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/llmProviders/mainProcessStream.ts` around lines 70 - 87, The
stream cleanup around the abort listener must remove the registered abort
handler when the stream finishes, not only rely on `{ once: true }`. Update the
cleanup/finally path for the listener and `abort` symbols to call
removeEventListener on the same signal, preserving cancellation behavior while
preventing stale http:stream-cancel requests.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/setup-embeddings.js (1)
88-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle response-stream failures after a successful status code.
Handle
responseerrorandabortedevents. Without handlers, the promise can remain pending, and an unhandlederrorevent can terminate the process. Destroy both streams, removedest, and reject once.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/setup-embeddings.js` around lines 88 - 106, Update the download flow around response.pipe(file) to handle response error and aborted events even after a successful status code. For either event, destroy both response and file streams, remove dest, and reject the promise exactly once; preserve the existing file-stream and request error cleanup while preventing duplicate rejection or cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-go-backend.yml:
- Around line 10-14: Update the pull_request.paths list in the workflow trigger
to include .github/workflows/build-go-backend.yml, so changes to the workflow
definition itself run backend validation before merge while preserving the
existing backend and embeddings path filters.
In `@electron/main/memorySearch.ts`:
- Around line 24-36: Update tokenizeMemoryQuery to use locale-independent
toLowerCase() instead of toLocaleLowerCase() before splitting tokens, ensuring
generated FTS tokens match unicode61 indexing across system locales.
In `@electron/main/windowManager.ts`:
- Around line 46-50: Update openExternal to attach a catch handler to the
Promise returned by shell.openExternal, preserving the existing URL validation
and warning behavior while handling asynchronous rejections without unhandled
Promise errors.
In `@electron/preload/ipcBridgePolicy.ts`:
- Line 41: Restrict the desktop:listDirectory bridge and its desktopManager
handling to an approved-directory capability: require a user-approved root,
resolve each renderer-supplied path within that root, and pass only the
validated resolved path to fs.readdir. Do not expose desktop:listDirectory when
this enforcement is unavailable.
---
Outside diff comments:
In `@scripts/setup-embeddings.js`:
- Around line 88-106: Update the download flow around response.pipe(file) to
handle response error and aborted events even after a successful status code.
For either event, destroy both response and file streams, remove dest, and
reject the promise exactly once; preserve the existing file-stream and request
error cleanup while preventing duplicate rejection or cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 285e07ef-a663-4cf9-ab21-676765763613
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.github/workflows/build-go-backend.ymlbackend/internal/api/embeddings.gobackend/internal/api/embeddings_test.gobackend/internal/minilm/onnx_embeddings.gobackend/internal/minilm/tokenizer_test.goelectron/main/desktopManager.tselectron/main/ipcManager.tselectron/main/memoryManager.tselectron/main/memorySearch.tselectron/main/ragDocumentStore.tselectron/main/securityBoundaries.tselectron/main/settingsManager.tselectron/main/thoughtVectorStore.tselectron/main/windowManager.tselectron/preload/index.tselectron/preload/ipcBridgePolicy.tspackage.jsonscripts/setup-embeddings.jssrc/__tests__/ipcBridgePolicy.test.tssrc/__tests__/memorySearch.test.tssrc/__tests__/securityBoundaries.test.tssrc/__tests__/settingsSecurity.test.tssrc/components/Chat.vuesrc/composables/useAudioProcessing.tssrc/utils/functions/filesystem.ts
💤 Files with no reviewable changes (1)
- src/components/Chat.vue
🚧 Files skipped from review as they are similar to previous changes (15)
- backend/internal/api/embeddings_test.go
- src/tests/settingsSecurity.test.ts
- src/tests/securityBoundaries.test.ts
- package.json
- electron/preload/index.ts
- src/tests/ipcBridgePolicy.test.ts
- src/utils/functions/filesystem.ts
- electron/main/settingsManager.ts
- src/composables/useAudioProcessing.ts
- electron/main/securityBoundaries.ts
- electron/main/memoryManager.ts
- backend/internal/api/embeddings.go
- backend/internal/minilm/onnx_embeddings.go
- electron/main/ipcManager.ts
- electron/main/ragDocumentStore.ts
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - 'backend/**' | ||
| - 'scripts/setup-embeddings.js' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run this workflow when its PR definition changes.
pull_request.paths does not include .github/workflows/build-go-backend.yml. A pull request that only changes this workflow will not run the backend validation before merge.
Proposed fix
pull_request:
branches: [main]
paths:
- 'backend/**'
- 'scripts/setup-embeddings.js'
+ - '.github/workflows/build-go-backend.yml'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request: | |
| branches: [main] | |
| paths: | |
| - 'backend/**' | |
| - 'scripts/setup-embeddings.js' | |
| pull_request: | |
| branches: [main] | |
| paths: | |
| - 'backend/**' | |
| - 'scripts/setup-embeddings.js' | |
| - '.github/workflows/build-go-backend.yml' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-go-backend.yml around lines 10 - 14, Update the
pull_request.paths list in the workflow trigger to include
.github/workflows/build-go-backend.yml, so changes to the workflow definition
itself run backend validation before merge while preserving the existing backend
and embeddings path filters.
1 participant
Summary
Why
The desktop runtime had accumulated stale dependencies and overly broad renderer capabilities. Local retrieval also performed poorly outside English, while VAD startup and cancellation contained packaging-specific race conditions.
Impact
Validation
npm test— 25 files, 111 testsnpx vue-tsc --noEmitnpm run build:webnpm audit— 0 vulnerabilitiesgo test ./...go vet ./...git diff --checkSummary by CodeRabbit
Summary by CodeRabbit