fix(windows): keep Windows ONNX, storage, and file-replacement fixes#66
Conversation
Per maintainer review, drops the findHighestCachedVersion resolver feature (shipped upstream in v0.31.1) and the doctor.ts/opencode.ts refactors. Keeps: - pre_validate_onnx_runtime: validate via LoadLibraryExW on Windows - configure: fs::create_dir_all(storage_dir) during configure - onnx-runtime + onnx: NuGet cache scan, PATH discovery, case-insensitive dedup - downloader + resolver: unlinkSync before renameSync (EEXIST fix) - diagnostics: best-effort storage mkdirSync for clearer doctor output
|
@cortexkit/maintainer Here's the rebased version per your review: Dropped (3 hunks exactly as requested):
Kept (7 files, +201/-44):
Verified: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 7 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Hey Zir, can you address cubic findings, after that we're ready to merge. Thanks! |
- P1 security: reuse pathEntriesForPlatform() in onnx-runtime.ts instead of raw PATH split (validates absolute paths, null bytes, quotes) - P2 security: add isAbsolute filter to onnx.ts PATH discovery - P2 version check: probe onnxruntime.dll version via GetFileVersionInfoW on Windows, matching the Linux/macOS version gate - P3 comments: fix misleading 'Atomic rename' labels in resolver.ts and downloader.ts
|
Addressed all 5 cubic findings (commit ea08fcb):
Verified: |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/aft/src/semantic_index.rs">
<violation number="1" location="crates/aft/src/semantic_index.rs:875">
P1: Windows version probing uses a fixed 260-char buffer for GetModuleFileNameW, which can truncate long DLL paths. When truncated, GetFileVersionInfoSizeW fails silently, detected_major stays 0, and the version check is bypassed — allowing incompatible ONNX DLLs to pass pre-validation and fail later in ort.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let mut path_buf = [0u16; 260]; | ||
| let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 260); |
There was a problem hiding this comment.
P1: Windows version probing uses a fixed 260-char buffer for GetModuleFileNameW, which can truncate long DLL paths. When truncated, GetFileVersionInfoSizeW fails silently, detected_major stays 0, and the version check is bypassed — allowing incompatible ONNX DLLs to pass pre-validation and fail later in ort.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/aft/src/semantic_index.rs, line 875:
<comment>Windows version probing uses a fixed 260-char buffer for GetModuleFileNameW, which can truncate long DLL paths. When truncated, GetFileVersionInfoSizeW fails silently, detected_major stays 0, and the version check is bypassed — allowing incompatible ONNX DLLs to pass pre-validation and fail later in ort.</comment>
<file context>
@@ -824,7 +867,58 @@ pub fn pre_validate_onnx_runtime() -> Result<(), String> {
+ // outdated DLLs (e.g. v1.9.x) before the ort crate panics.
+ let mut detected_major: u32 = 0;
+ let mut detected_minor: u32 = 0;
+ let mut path_buf = [0u16; 260];
+ let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 260);
+ if path_len > 0 {
</file context>
| let mut path_buf = [0u16; 260]; | |
| let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 260); | |
| let mut path_buf = [0u16; 32768]; | |
| let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 32768); |
Zireael
force-pushed
the
fix/windows-doctor-issue-64
branch
from
94cf0fb to
ea08fcb
Compare
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/docker-rust.ps1">
<violation number="1">
P3: Run-Autofmt unconditionally reports success even when the underlying Docker/cargo command fails</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
@Zireael Waiting for fixes so that tests passes. |
|
Also if you're using discord please join us: https://discord.gg/DSa65w8wuf |
…_mismatch compile error and Linux case-sensitive existsSync
Will do. Edit: tests all green /tearsofjoy |
Yes 0.32 released with unified aft_search, after this feature is polished we'll probably advice removing grep from agent's toolbox. (at least 10-15 minors later). Also cleaned up/trimmed the tool/param descriptions. Aim is to use the least context while providing highest return. |
- Increase GetModuleFileNameW buffer from 260 to 32767 (MAX_UNICODEPATH) to prevent silent truncation on deeply nested Windows paths - Remove unreachable #[cfg(target_os = "windows")] dead code in suggest_removal_command (Windows paths never start with /usr/local/lib) - Remove duplicate PATH scanning loop in CLI onnx.ts (pathEntriesForPlatform() already handles PATH) - Replace mkdirSync side effect in diagnostics.ts with read-only access check (doctor should not mutate filesystem) - Normalize Windows path comparisons to lowercase for case-insensitive matching in bridge onnx-runtime.ts - Add warning logging for NuGet scan failures in bridge onnx-runtime.ts - Fix cubic finding: replace unlink-then-rename with copyFileSync+unlink in downloader.ts to eliminate the window where no binary exists
Pre-existing rustfmt drift inherited from PR #66's Windows ONNX version-check addition. Applied with cargo fmt -p agent-file-tools. No behavioral change.
2 participants
Rebased on latest
cortexkit/main(post-v0.31.1) per maintainer review.Dropped (shipped in v0.31.1 or conflicting)
findHighestCachedVersion+compareSemverinresolver.ts— superseded by v0.31.1 skew detectionopencode.tsgetStorageDirrewrite — upstream already routes throughgetCortexKitStorageRootdoctor.tsissue bullets +log.successoutro — equivalent shipped in v0.31.1Kept (7 files, +201/-44)
crates/aft/src/semantic_index.rspre_validate_onnx_runtimevalidates viaLoadLibraryExWon Windows + updated install hintortcrate panicscrates/aft/src/commands/configure.rsfs::create_dir_all(storage_dir)during configurepackages/aft-bridge/src/onnx-runtime.ts%PATH%discovery + case-insensitive dedup + PATH-entry version-skippackages/aft-cli/src/lib/onnx.tspackages/aft-bridge/src/downloader.tsunlinkSyncbeforerenameSyncon Windowspackages/aft-bridge/src/resolver.tsunlinkSyncbeforerenameSyncincopyToVersionedCachepackages/aft-cli/src/lib/diagnostics.tsmkdirSyncfor storage dirVerification
npx tsc --noEmitinpackages/aft-bridgeandpackages/aft-cli— cleancargo check --workspace --all-targets— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleanSummary by cubic
Improves Windows reliability: pre-validates and version-checks
onnxruntime.dll, hardens ONNX discovery, and fixes file replacement crashes. Also ensures the storage directory exists during configure and diagnostics.Bug Fixes
onnxruntime.dllviaLoadLibraryExW+GetFileVersionInfoW; clearer errors and stale DLL rejection; updated install hint withnpx @cortexkit/aft doctor.renameSyncto prevent EEXIST when replacing binaries indownloader.tsandresolver.ts.create_dir_allin Rust configure; best‑effortmkdirSyncin CLI diagnostics.pathEntriesForPlatform()in the bridge and require absolute PATH entries in the CLI; fix Windows build forformat_ort_version_mismatchand Linux case‑sensitive path checks.New Features
%USERPROFILE%\.nuget\packages\microsoft.ml.onnxruntime\...), common Program Files paths, and absolute%PATH%; case‑insensitive dedup; skip version checks for PATH entries while still filtering stale installs elsewhere.Written for commit d0a3231. Summary will update on new commits. Review in cubic