V3.1.2 beta.rc2#249
Conversation
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
- OVMS pinned to weekly build 2026.3.0.8022ddae3 (releaseTag 39717a69b → 8022ddae3). - Added a sync helper to read a backend's version/releaseTag from the shipped backend-versions.json. - Removed OVMS's hardcoded version/releaseTag defaults, sourcing them from the JSON instead, and added a guard against an unset version during download. - Fixed package author name/email (Intel® Corporation/TODO → Intel Corporation/webadmin@linux.intel.com). - Lowered the Phison preset contextSize from 64000 to 8192. Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
Signed-off-by: Leslie Lazzarino <leslie.lazzarino@tngtech.com>
📝 WalkthroughWalkthroughThis PR adds pre-commit hook infrastructure ( ChangesPre-commit Hook Setup
OpenVINO Backend Service Changes
Live Reasoning Display
Data URI Image Downscaling
Sequence Diagram(s)sequenceDiagram
participant BackendOptions.vue
participant backendServices
participant IPC as updateServiceSettings IPC
participant OpenVINOBackendService
BackendOptions.vue->>backendServices: openvinoKvCacheU4 = true
backendServices->>IPC: ovmsKvCachePrecision = 'u4'
IPC->>OpenVINOBackendService: updateSettings({ovmsKvCachePrecision: 'u4'})
OpenVINOBackendService->>OpenVINOBackendService: stopOvmsLlmServer + stopOvmsEmbeddingServer
Note over OpenVINOBackendService: kvCachePrecision = 'u4'
OpenVINOBackendService->>OpenVINOBackendService: startOvmsLlmServer(--kv_cache_precision u4)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
WebUI/src/assets/js/store/homeAgent.ts (1)
1131-1138: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
mediaType/filenameextension can diverge from the downscaled bytes.
downscaleDataUriTo1MPre-encodes any source that isn't JPEG/WebP to PNG (viaresolveDownscaleEncoding). For an oversized source whoseimg.mimeis e.g.image/gif/image/bmp/image/avif/image/heic, the persisted bytes become PNG whilemediaTypeand theextstill report the original type, so the file part advertises a MIME that no longer matches its content. Common channel formats (jpeg/png/webp) are preserved, so impact is limited to exotic inputs.Consider deriving the type from the downscaled URI rather than the original
img.mime:♻️ Proposed adjustment
- const downscaledUri = await downscaleDataUriTo1MP(dataUri) - const aipgUrl = await saveImageToMediaInput(downscaledUri) - const ext = img.mime === 'image/jpeg' ? 'jpg' : img.mime.replace('image/', '') + const downscaledUri = await downscaleDataUriTo1MP(dataUri) + const aipgUrl = await saveImageToMediaInput(downscaledUri) + const effectiveMime = downscaledUri.match(/^data:([^;,]+)[;,]/)?.[1] ?? img.mime + const ext = effectiveMime === 'image/jpeg' ? 'jpg' : effectiveMime.replace('image/', '') parts.push({ type: 'file', - mediaType: img.mime, + mediaType: effectiveMime, url: aipgUrl, filename: `${sourceLabel}-${Date.now()}-${i}.${ext}`, })🤖 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 `@WebUI/src/assets/js/store/homeAgent.ts` around lines 1131 - 1138, The file part in homeAgent.ts is using the original img.mime to set mediaType and filename extension, but downscaleDataUriTo1MP can re-encode some inputs to PNG, so the advertised type may not match the saved bytes. Update the logic around saveImageToMediaInput and the parts.push payload to derive the output MIME/extension from the downscaled data URI (or from the downscale helper’s chosen encoding) instead of img.mime, while keeping the existing behavior for JPEG/WebP and matching the filename suffix to the actual persisted format.WebUI/electron/subprocesses/openVINOBackendService.ts (1)
1724-1749: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSurface stop failures from
stop().
stopAllModelServers()swallows every exception, thenstop()always returns'stopped'.apiServiceRegistry.stopAllServices()will treat that as success, so leaked OVMS workers can keep ports or files locked while the app reports a clean shutdown. Please propagate an aggregate failure here and keep the best-effort behavior only for the reinstall path if that's intentional.🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1724 - 1749, `stopAllModelServers()` is currently swallowing all stop errors, so `stop()` always reports a clean shutdown even when one or more OVMS servers fail to stop; update `stopAllModelServers()` and the `stop()` flow in `openVINOBackendService` to collect any failures and surface an aggregate error or failure status back to the caller instead of always returning success. Keep the existing best-effort per-server shutdown behavior, but ensure `stop()` can fail when any of the `stopOvmsLlmServer`, `stopOvmsEmbeddingServer`, `stopOvmsTranscriptionServer`, `stopOvmsSpeechServer`, or `stopOvmsImageServer` calls fail so `apiServiceRegistry.stopAllServices()` can detect the failure.
🧹 Nitpick comments (1)
WebUI/src/env.d.ts (1)
72-73: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNarrow
ovmsKvCachePrecisionto the supported literals.This IPC contract currently accepts any string, but
WebUI/electron/subprocesses/openVINOBackendService.tsforwards non-empty values straight into--kv_cache_precision. Typing it as'' | 'u8' | 'u4' | 'f16' | 'fp32'keeps renderer and main in sync and prevents invalid launch args from compiling.Proposed fix
+type OvmsKvCachePrecision = '' | 'u8' | 'u4' | 'f16' | 'fp32' + type ServiceSettings = { serviceName: BackendServiceName version?: string releaseTag?: string comfyUiParameters?: string llamaCppParameters?: string llamaCppBuildVariant?: 'standard' | 'ssd-offload' llamaCppOffloadDrive?: string | null // OVMS --kv_cache_precision value ('u8' | 'u4' | 'f16' | 'fp32'); '' = OVMS default. - ovmsKvCachePrecision?: string + ovmsKvCachePrecision?: OvmsKvCachePrecision }🤖 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 `@WebUI/src/env.d.ts` around lines 72 - 73, The IPC contract for ovmsKvCachePrecision is too broad and should be narrowed to the supported literals. Update the type in env.d.ts so it only allows '' | 'u8' | 'u4' | 'f16' | 'fp32', matching what openVINOBackendService forwards into --kv_cache_precision, and keep the renderer/main contract in sync so invalid launch arguments are rejected at compile time.
🤖 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 @.pre-commit-config.yaml:
- Around line 26-44: The formatter hooks are still treated as failures when they
modify files, so the current ruff-format and prettier entries in the pre-commit
config will reject the first commit after reformatting. Update the hook setup
for ruff-format and prettier so they use a pre-commit-compatible
format-and-restage approach (for example via a proper wrapper or
pre-commit-managed stages) rather than relying on plain git add after
formatting, and keep the existing hook names/files patterns so they remain easy
to locate.
In `@WebUI/build/scripts/ensure-precommit-hook.mjs`:
- Around line 71-84: The fallback in ensure-precommit-hook.mjs only checks
ambient PATH for pre-commit and uvx, so it misses the uv binary downloaded by
setup. Update the runner selection logic in ensure-precommit-hook.mjs to detect
and use the repo-provided uv toolchain before falling back to system commands,
then invoke pre-commit through that binary when available. Keep the existing
commandAvailable flow and preserve the current user-facing failure message only
for the true no-tool case.
In `@WebUI/electron/subprocesses/openVINOBackendService.ts`:
- Around line 2168-2177: Keep the OVMS process object tracked until exit is
actually confirmed. In `terminateProcessTree`, when the second `waitForExit()`
times out, do not just log and resolve; return a failure signal (or otherwise
indicate the process is still alive) so `stopOvmsLlmServer` can avoid clearing
`this.ovmsLlmProcess` prematurely. Update `stopOvmsLlmServer` and the
`terminateProcessTree` flow to preserve `this.ovmsLlmProcess` until the `exit`
event is observed.
- Around line 1594-1602: Scope the Windows cleanup in
killStrayOvmsProcessesWindows so it only targets OVMS processes started by this
service instead of using a system-wide taskkill on ovms.exe. Update the logic to
identify and terminate only tracked child PIDs or processes whose executable
path is under this.ovmsDir, and keep the existing logging in
openVINOBackendService aligned with the narrowed scope. Ensure the change
preserves the current non-fatal handling when nothing matching is found.
- Around line 1247-1255: The version marker is currently written from the
requested values instead of the actual downloaded artifact metadata, which can
make getInstalledVersion() report the wrong release tag. Update
openVINOBackendService’s download flow so the selected artifact metadata is
captured when the download URL is chosen in downloadOvms(), then have
writeVersionMarker() persist that stored artifact version/releaseTag instead of
this.version/this.releaseTag. Keep the fix localized to the download selection
and writeVersionMarker() path so the marker always reflects what was actually
installed.
In `@WebUI/src/assets/js/store/backendServices.ts`:
- Around line 419-435: The OpenVINO KV-cache sync failure is only logged with
console.warn inside the watch handler for openvinoKvCacheU4, so it bypasses the
app’s error pipeline. Update that catch path to route the caught error through
the errors store via errors.report(err, overrides?) instead of warning directly,
using a non-blocking surface such as silent or warn. Keep the sync logic in
backendServices.ts and the updateServiceSettings call intact, but ensure
failures are reported consistently through useErrors.
In `@WebUI/src/assets/js/store/openAiCompatibleChat.ts`:
- Around line 96-109: The reasoning stream state in openAiCompatibleChat.ts is
currently shared globally via reasoningInProgress and reasoningStartedAt, so a
generate() call for another conversationKey can overwrite the active chat’s
timer state. Scope these refs by conversationKey inside the store and update the
generate()/chunk-stream handling to read and write per-conversation state, then
expose the active conversation’s derived reasoning values to Chat.vue instead of
a single shared pair.
---
Outside diff comments:
In `@WebUI/electron/subprocesses/openVINOBackendService.ts`:
- Around line 1724-1749: `stopAllModelServers()` is currently swallowing all
stop errors, so `stop()` always reports a clean shutdown even when one or more
OVMS servers fail to stop; update `stopAllModelServers()` and the `stop()` flow
in `openVINOBackendService` to collect any failures and surface an aggregate
error or failure status back to the caller instead of always returning success.
Keep the existing best-effort per-server shutdown behavior, but ensure `stop()`
can fail when any of the `stopOvmsLlmServer`, `stopOvmsEmbeddingServer`,
`stopOvmsTranscriptionServer`, `stopOvmsSpeechServer`, or `stopOvmsImageServer`
calls fail so `apiServiceRegistry.stopAllServices()` can detect the failure.
In `@WebUI/src/assets/js/store/homeAgent.ts`:
- Around line 1131-1138: The file part in homeAgent.ts is using the original
img.mime to set mediaType and filename extension, but downscaleDataUriTo1MP can
re-encode some inputs to PNG, so the advertised type may not match the saved
bytes. Update the logic around saveImageToMediaInput and the parts.push payload
to derive the output MIME/extension from the downscaled data URI (or from the
downscale helper’s chosen encoding) instead of img.mime, while keeping the
existing behavior for JPEG/WebP and matching the filename suffix to the actual
persisted format.
---
Nitpick comments:
In `@WebUI/src/env.d.ts`:
- Around line 72-73: The IPC contract for ovmsKvCachePrecision is too broad and
should be narrowed to the supported literals. Update the type in env.d.ts so it
only allows '' | 'u8' | 'u4' | 'f16' | 'fp32', matching what
openVINOBackendService forwards into --kv_cache_precision, and keep the
renderer/main contract in sync so invalid launch arguments are rejected at
compile time.
🪄 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
Run ID: 01a2a78c-54d3-4624-aa55-0be5df9722f3
📒 Files selected for processing (17)
.pre-commit-config.yamlWebUI/build/scripts/ensure-precommit-hook.mjsWebUI/electron/remoteUpdates.tsWebUI/electron/subprocesses/openVINOBackendService.tsWebUI/external/backend-versions.jsonWebUI/package.jsonWebUI/src/assets/i18n/en-US.jsonWebUI/src/assets/js/store/backendServices.tsWebUI/src/assets/js/store/homeAgent.tsWebUI/src/assets/js/store/openAiCompatibleChat.tsWebUI/src/components/BackendOptions.vueWebUI/src/components/ChatReasoningDisplay.vueWebUI/src/env.d.tsWebUI/src/lib/utils.tsWebUI/src/views/Chat.vuemodes/base/presets/phison.jsonreadme.md
| # Formatter (Ruff) — format in place, restage, never fail on reformat | ||
| - id: ruff-format | ||
| name: ruff format (Python) | ||
| language: system | ||
| entry: bash -c 'uvx ruff@0.15.20 format "$@" && git add -- "$@"' -- | ||
| files: ^(service|home-agent)/.*\.py$ | ||
| # Linter (ESLint with auto-fix) | ||
| - id: eslint | ||
| name: eslint (WebUI) | ||
| language: system | ||
| entry: bash -c 'cd WebUI && npm run lint:eslint' | ||
| files: ^WebUI/.*\.(ts|tsx|mts|cts|js|jsx|mjs|cjs|vue)$ | ||
| pass_filenames: false | ||
| # Formatter (Prettier) — format in place, restage, never fail on reformat | ||
| - id: prettier | ||
| name: prettier (WebUI) | ||
| language: system | ||
| entry: bash -c 'cd WebUI && files=(); for f in "$@"; do files+=("${f#WebUI/}"); done && npx prettier --write "${files[@]}" && cd .. && git add -- "$@"' -- | ||
| files: ^WebUI/.*\.(ts|tsx|mts|cts|js|jsx|mjs|cjs|vue|json|css|scss|html|md|ya?ml)$ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does pre-commit mark a hook as failed when it modifies files, even if the hook exits successfully and restages the files with git add?
💡 Result:
Yes, pre-commit marks a hook as failed if it modifies files, even if the hook exits with a successful (zero) exit code [1][2][3]. The pre-commit framework explicitly considers a hook to have failed if either it returns a non-zero exit code or if it modifies files during its execution [1][2]. The framework detects these modifications by comparing the state of the repository before and after the hook runs using git diff [4][2]. This behavior is a deliberate design choice rather than a bug [5][4][2]. The rationale is that if a hook modifies files, the commit is no longer in the state the user intended to commit, and the framework intentionally forces the user to review the changes, manually stage them (if desired), and re-commit [5][6]. Attempting to automatically stage files modified by a hook is intentionally not supported, as it could lead to unexpected behavior, such as accidentally staging unrelated unstaged changes, overwriting intentional manual changes, or committing code that hasn't been properly vetted by the human user [5][6]. If you use a hook that modifies files (like a code formatter), the standard workflow is to let the hook fail, then stage the modified files and run the commit again [5][6][7]. Because the changes are now in the staging area, the next attempt will typically pass, provided the formatter is consistent and doesn't introduce further changes [5][2].
Citations:
- 1: https://pre-commit.com/
- 2: Conflicting pre-commit rewrites prevent commit pre-commit/pre-commit#1104
- 3: Add pre-commit hook configuration option to always re-stage modified files so pre-commit doesn't fail psf/black#1857
- 4: Add modified files to the index. pre-commit/pre-commit#2346
- 5: Hook that updates files pre-commit/pre-commit#532
- 6: Automatically stage files changed by hook pre-commit/pre-commit#806
- 7: https://stackoverflow.com/questions/58398995/black-as-pre-commit-hook-always-fails-my-commits
git add won’t stop pre-commit from failing on formatter changes. pre-commit still marks a hook as failed when it modifies tracked files, even if it exits 0 and restages them, so the first formatting pass will reject the commit. If the goal is a single-pass format-and-commit flow, this needs a different integration.
🤖 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 @.pre-commit-config.yaml around lines 26 - 44, The formatter hooks are still
treated as failures when they modify files, so the current ruff-format and
prettier entries in the pre-commit config will reject the first commit after
reformatting. Update the hook setup for ruff-format and prettier so they use a
pre-commit-compatible format-and-restage approach (for example via a proper
wrapper or pre-commit-managed stages) rather than relying on plain git add after
formatting, and keep the existing hook names/files patterns so they remain easy
to locate.
| // Prefer a `pre-commit` on PATH; otherwise run it through uv's `uvx`. | ||
| let runner | ||
| if (commandAvailable('pre-commit')) { | ||
| runner = ['pre-commit'] | ||
| } else if (commandAvailable('uvx')) { | ||
| runner = ['uvx', 'pre-commit'] | ||
| } else { | ||
| log('`pre-commit` is not installed and `uvx` is unavailable, so the hook was not installed.') | ||
| log('Install it once with either of:') | ||
| log(' uv tool install pre-commit') | ||
| log(' pipx install pre-commit') | ||
| log('then re-run `npm run setup` (or `pre-commit install`).') | ||
| process.exit(0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This never uses the repo-downloaded uv toolchain.
npm run setup fetches uv immediately before invoking this script, but this fallback only checks pre-commit and uvx on the ambient PATH. Fresh clones without a global install will still hit the “unavailable” branch instead of using the downloaded binary, so the auto-install path is weaker than the README now promises.
🤖 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 `@WebUI/build/scripts/ensure-precommit-hook.mjs` around lines 71 - 84, The
fallback in ensure-precommit-hook.mjs only checks ambient PATH for pre-commit
and uvx, so it misses the uv binary downloaded by setup. Update the runner
selection logic in ensure-precommit-hook.mjs to detect and use the repo-provided
uv toolchain before falling back to system commands, then invoke pre-commit
through that binary when available. Keep the existing commandAvailable flow and
preserve the current user-facing failure message only for the true no-tool case.
| private async writeVersionMarker(): Promise<void> { | ||
| try { | ||
| await filesystem.writeJson(this.versionMarkerPath, { | ||
| version: this.version, | ||
| ...(this.releaseTag && { releaseTag: this.releaseTag }), | ||
| }) | ||
| } catch (e) { | ||
| this.appLogger.warn(`Failed to write OpenVINO version marker: ${e}`, this.name) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist the downloaded artifact metadata, not the requested values.
writeVersionMarker() currently serializes this.version / this.releaseTag, but those are just the requested settings. downloadOvms() can fall back to a different asset, and getInstalledVersion() now trusts this marker first, so the app can report a release tag that was never actually installed. Capture the selected artifact metadata when the download URL is chosen and write that instead.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1247 -
1255, The version marker is currently written from the requested values instead
of the actual downloaded artifact metadata, which can make getInstalledVersion()
report the wrong release tag. Update openVINOBackendService’s download flow so
the selected artifact metadata is captured when the download URL is chosen in
downloadOvms(), then have writeVersionMarker() persist that stored artifact
version/releaseTag instead of this.version/this.releaseTag. Keep the fix
localized to the download selection and writeVersionMarker() path so the marker
always reflects what was actually installed.
| private async killStrayOvmsProcessesWindows(): Promise<void> { | ||
| if (process.platform !== 'win32') return | ||
| try { | ||
| await execAsync('taskkill /F /IM ovms.exe /T') | ||
| this.appLogger.info('Killed stray ovms.exe processes before extraction', this.name) | ||
| } catch (e) { | ||
| // Exits non-zero when no ovms.exe is running — expected and not fatal. | ||
| this.appLogger.info(`No stray ovms.exe to kill (or taskkill reported: ${e})`, this.name) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't kill every ovms.exe on the machine.
taskkill /IM ovms.exe /T /F is system-wide. During setup it will also terminate unrelated OVMS instances the user started outside AI Playground, which makes reinstall destructive for other local workloads. Scope this to processes owned by this service (tracked child PIDs, or processes whose executable path lives under this.ovmsDir).
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 1594 -
1602, Scope the Windows cleanup in killStrayOvmsProcessesWindows so it only
targets OVMS processes started by this service instead of using a system-wide
taskkill on ovms.exe. Update the logic to identify and terminate only tracked
child PIDs or processes whose executable path is under this.ovmsDir, and keep
the existing logging in openVINOBackendService aligned with the narrowed scope.
Ensure the change preserves the current non-fatal handling when nothing matching
is found.
| exited = await waitForExit(5000) | ||
| if (!exited) { | ||
| this.appLogger.warn(`OVMS ${label} not confirmed exited after force kill`, this.name) | ||
| } | ||
| } | ||
|
|
||
| private async stopOvmsLlmServer(): Promise<void> { | ||
| if (this.ovmsLlmProcess) { | ||
| this.appLogger.info(`Stopping OVMS LLM server for model: ${this.currentModel}`, this.name) | ||
| this.ovmsLlmProcess.process.kill('SIGTERM') | ||
|
|
||
| // Wait a bit for graceful shutdown, then force kill if needed | ||
| await new Promise<void>((resolve) => { | ||
| const currentProcess = this.ovmsLlmProcess | ||
| const timeout = setTimeout(() => { | ||
| if (currentProcess) { | ||
| this.appLogger.warn(`Force killing OVMS LLM server process`, this.name) | ||
| currentProcess.process.kill('SIGKILL') | ||
| } | ||
| resolve() | ||
| }, 5000) | ||
|
|
||
| if (currentProcess) { | ||
| currentProcess.process.on('exit', () => { | ||
| clearTimeout(timeout) | ||
| resolve() | ||
| }) | ||
| } else { | ||
| clearTimeout(timeout) | ||
| resolve() | ||
| } | ||
| }) | ||
| await this.terminateProcessTree(this.ovmsLlmProcess.process, 'LLM server') |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the process tracked until exit is actually confirmed.
When the second waitForExit() times out, terminateProcessTree() only logs and returns. stopOvmsLlmServer() then clears this.ovmsLlmProcess anyway, so an unresponsive server can stay alive after we've dropped the last reference to it. Return a failure here instead of resolving, or keep the process object until the exit event fires.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { ChildProcess, spawn } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@WebUI/electron/subprocesses/openVINOBackendService.ts` around lines 2168 -
2177, Keep the OVMS process object tracked until exit is actually confirmed. In
`terminateProcessTree`, when the second `waitForExit()` times out, do not just
log and resolve; return a failure signal (or otherwise indicate the process is
still alive) so `stopOvmsLlmServer` can avoid clearing `this.ovmsLlmProcess`
prematurely. Update `stopOvmsLlmServer` and the `terminateProcessTree` flow to
preserve `this.ovmsLlmProcess` until the `exit` event is observed.
| /** Sync the OpenVINO KV cache precision toggle to the main process so a | ||
| * running OVMS service reloads its chat server with the new precision. */ | ||
| watch( | ||
| openvinoKvCacheU4, | ||
| async () => { | ||
| try { | ||
| await updateServiceSettings({ | ||
| serviceName: 'openvino-backend', | ||
| ovmsKvCachePrecision: effectiveOvmsKvCachePrecision.value, | ||
| }) | ||
| } catch (e) { | ||
| console.warn('Failed to sync OpenVINO KV cache precision to main process:', e) | ||
| } | ||
| }, | ||
| { flush: 'post' }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Report KV-cache sync failures through the errors store.
If this IPC update fails, the renderer toggle changes while the main process keeps the old precision. Logging a warning leaves that mismatch outside the app’s error pipeline; route it through errors.report(...) with a silent or warn surface instead.
As per coding guidelines, "All errors must flow through the errors store (useErrors). Report errors via errors.report(err, overrides?)" and "Use surface control in errors: 'toast' (default, user-facing), 'inline', 'modal', or 'silent' (background work, no UI)."
Proposed fix
+import { createAppError } from '`@/assets/js/errors/appError`'
+import { useErrors } from '`@/assets/js/store/errors`'
+
export const useBackendServices = defineStore(
'backendServices',
() => {
+ const errors = useErrors()
+
// ...
watch(
openvinoKvCacheU4,
async () => {
try {
await updateServiceSettings({
serviceName: 'openvino-backend',
ovmsKvCachePrecision: effectiveOvmsKvCachePrecision.value,
})
} catch (e) {
- console.warn('Failed to sync OpenVINO KV cache precision to main process:', e)
+ errors.report(
+ createAppError({
+ category: 'backend',
+ code: 'openvino/kv-cache-sync-failed',
+ userMessage: 'Failed to sync the OpenVINO KV cache setting.',
+ surface: 'silent',
+ severity: 'warn',
+ cause: e,
+ }),
+ )
}
},
{ flush: 'post' },
)📝 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.
| /** Sync the OpenVINO KV cache precision toggle to the main process so a | |
| * running OVMS service reloads its chat server with the new precision. */ | |
| watch( | |
| openvinoKvCacheU4, | |
| async () => { | |
| try { | |
| await updateServiceSettings({ | |
| serviceName: 'openvino-backend', | |
| ovmsKvCachePrecision: effectiveOvmsKvCachePrecision.value, | |
| }) | |
| } catch (e) { | |
| console.warn('Failed to sync OpenVINO KV cache precision to main process:', e) | |
| } | |
| }, | |
| { flush: 'post' }, | |
| ) | |
| import { createAppError } from '`@/assets/js/errors/appError`' | |
| import { useErrors } from '`@/assets/js/store/errors`' | |
| export const useBackendServices = defineStore( | |
| 'backendServices', | |
| () => { | |
| const errors = useErrors() | |
| /** Sync the OpenVINO KV cache precision toggle to the main process so a | |
| * running OVMS service reloads its chat server with the new precision. */ | |
| watch( | |
| openvinoKvCacheU4, | |
| async () => { | |
| try { | |
| await updateServiceSettings({ | |
| serviceName: 'openvino-backend', | |
| ovmsKvCachePrecision: effectiveOvmsKvCachePrecision.value, | |
| }) | |
| } catch (e) { | |
| errors.report( | |
| createAppError({ | |
| category: 'backend', | |
| code: 'openvino/kv-cache-sync-failed', | |
| userMessage: 'Failed to sync the OpenVINO KV cache setting.', | |
| surface: 'silent', | |
| severity: 'warn', | |
| cause: e, | |
| }), | |
| ) | |
| } | |
| }, | |
| { flush: 'post' }, | |
| ) |
🤖 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 `@WebUI/src/assets/js/store/backendServices.ts` around lines 419 - 435, The
OpenVINO KV-cache sync failure is only logged with console.warn inside the watch
handler for openvinoKvCacheU4, so it bypasses the app’s error pipeline. Update
that catch path to route the caught error through the errors store via
errors.report(err, overrides?) instead of warning directly, using a non-blocking
surface such as silent or warn. Keep the sync logic in backendServices.ts and
the updateServiceSettings call intact, but ensure failures are reported
consistently through useErrors.
Source: Coding guidelines
| // True while the model is actively emitting reasoning (i.e. the last content | ||
| // chunk was a reasoning delta and no text/tool chunk has followed). Driven | ||
| // straight off the chunk stream so the UI never has to infer "is reasoning | ||
| // still going?" from part positions or the per-delta `reasoningFinished` | ||
| // timestamp (which is bumped to "now" on every delta and so always looks | ||
| // recent). Cleared when the turn ends (see the `processing` safety-net watch). | ||
| const reasoningInProgress = ref(false) | ||
| // Wall-clock start of the reasoning block currently in progress. The part's | ||
| // own `reasoningStarted` metadata isn't attached to the live (still- | ||
| // streaming) UI part, so the chat view needs this to drive an increasing | ||
| // "Reasoned for X.Xs" timer; once the block finishes the view falls back to | ||
| // the part metadata. Updated to the block start whenever a new block begins. | ||
| const reasoningStartedAt = ref(0) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Scope reasoning stream state by conversation.
useHomeAgent can call generate() for a different conversationKey while the desktop chat is open, but these new refs are store-global. A background stream can therefore flip reasoningInProgress/reasoningStartedAt for the active chat, which makes Chat.vue show a live timer on the wrong reasoning block or with the wrong start time. Please key this state by conversationKey and expose the active conversation's derived values instead of sharing one global pair across all chats.
Also applies to: 684-696, 745-761, 806-806, 1173-1174
🤖 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 `@WebUI/src/assets/js/store/openAiCompatibleChat.ts` around lines 96 - 109, The
reasoning stream state in openAiCompatibleChat.ts is currently shared globally
via reasoningInProgress and reasoningStartedAt, so a generate() call for another
conversationKey can overwrite the active chat’s timer state. Scope these refs by
conversationKey inside the store and update the generate()/chunk-stream handling
to read and write per-conversation state, then expose the active conversation’s
derived reasoning values to Chat.vue instead of a single shared pair.
2 participants
Description:
Please provide a brief description of the changes made in this pull request.
Related Issue:
If this pull request is related to any existing issue, please mention the issue number here.
Changes Made:
Please list down the specific changes made in this pull request.
Testing Done:
Please describe the testing that has been done to ensure the changes made in this pull request are functioning as expected.
Screenshots:
If applicable, please provide screenshots or GIFs or videos to visually demonstrate the changes made.
Checklist:
Summary by CodeRabbit