Fix tray crash-loop: source x64 VC++ runtime from VS install at publish time - #713
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 7, 2026, 6:19 PM ET / 22:19 UTC. Summary Reproducibility: yes. The linked report at #703 plus current source give a high-confidence Windows x64 path: publish with the app-local 14.29 VC runtime, launch tray/TTS, and observe the Sherpa/ONNX initialization crash; I did not execute that Windows repro here. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land a narrow release fix that sources x64 VC runtime DLLs from a current Microsoft redist source, keeps native probes scoped to native DLLs, makes the x64 publish prerequisite an explicit maintainer-approved choice, and includes redacted Windows x64 publish/tray startup proof. Do we have a high-confidence way to reproduce the issue? Yes. The linked report at #703 plus current source give a high-confidence Windows x64 path: publish with the app-local 14.29 VC runtime, launch tray/TTS, and observe the Sherpa/ONNX initialization crash; I did not execute that Windows repro here. Is this the best way to solve the issue? No, not as submitted. The MSBuild direction is plausible, but the verifier should avoid native-probing the managed Sherpa wrapper and the PR needs real Windows x64 after-fix proof before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against e0a4b7c7c28d. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
…nnxruntime crash The VCRuntime.CefSharp.140 NuGet ships 14.29 DLLs that are too old for onnxruntime 1.26. When these sit app-locally they shadow the system runtime, causing sherpa-onnx-c-api to fail init (0x8007045A) and the OfflineTts finalizer to crash-loop. Both x64 and ARM64 publish now resolve VC++ runtime DLLs from the Visual Studio install via vswhere (ARM64 already did this). The NuGet is retained only as a dev-time convenience for local x64 builds. Also adds a minimum version floor (14.38) in the release validation script to prevent regressions. Co-authored-by: shanselman <2892+shanselman@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Validation/proof after the native TTS probe update:
|
Bundled
msvcp140.dll/vcruntime140.dllfromVCRuntime.CefSharp.140NuGet (v1.0.5) are 14.29 — too old foronnxruntime1.26. App-local DLL search order shadows the system runtime, sosherpa-onnx-c-apifails init (0x8007045A), theOfflineTtsfinalizer calls into the dead DLL, and the tray crash-loops on startup.Changes
src/Directory.Build.targets— GeneralizedResolveOpenClawVCRuntimeArm64FromVSInstall→ResolveOpenClawVCRuntimeFromVSInstall. x64 publish now resolves DLLs from VS install (same as ARM64 already did), replacing the stale NuGet items. NuGet retained only for localdotnet builddev convenience. Legacy alias target kept for backward compat.scripts/Test-ReleaseNativeDependencies.ps1— AddedAdd-VCRuntimeVersionFloorErrorswith a minimum version floor of14.38.33130.0(VS 2022 17.8, first CRT compatible with onnxruntime ≥ 1.20). Prevents silent regression to stale DLLs.tests/OpenClaw.Tray.Tests/ReleaseSigningWorkflowTests.cs— Assertions updated for new target name + version floor presence.docs/RELEASING.md— Updated to reflect both architectures now source from VS install.