chore: post-review fixes for PR #66

crates/aft/src/semantic_index.rs

@@ -872,8 +872,13 @@ 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);
+            // Use MAX_UNICODEPATH (32767) so deeply nested ORT paths (e.g.
+            // long NuGet package paths under %USERPROFILE%) never truncate.
+            // GetModuleFileNameW truncates silently when the buffer is too
+            // small, which causes version probing to fail and the version
+            // check to be bypassed — better to allocate generously.
+            let mut path_buf = [0u16; 32767];
+            let path_len = GetModuleFileNameW(handle, path_buf.as_mut_ptr(), 32767);
             if path_len > 0 {
                 let mut dummy_handle: u32 = 0;
                 let info_size =
@@ -978,8 +983,6 @@ fn suggest_removal_command(lib_path: &str) -> String {
         return "   sudo rm /usr/local/lib/libonnxruntime* && sudo ldconfig".to_string();
         #[cfg(target_os = "macos")]
         return "   sudo rm /usr/local/lib/libonnxruntime*".to_string();
-        #[cfg(target_os = "windows")]
-        return "   Delete the ONNX Runtime DLL from your PATH".to_string();
     }
     format!("   rm '{}'", lib_path)
 }

packages/aft-bridge/src/downloader.ts

@@ -11,21 +11,19 @@
  * Cache dir respects XDG_CACHE_HOME on Linux/macOS and LOCALAPPDATA on Windows.
  */
 
-import { spawnSync } from "node:child_process";
-import { createHash, randomUUID } from "node:crypto";
+import { createHash } from "node:crypto";
 import {
   chmodSync,
   closeSync,
+  copyFileSync,
   createWriteStream,
   existsSync,
   mkdirSync,
   openSync,
-  readFileSync,
   renameSync,
   rmSync,
   statSync,
   unlinkSync,
-  writeSync,
 } from "node:fs";
 import { homedir } from "node:os";
 import { join } from "node:path";
@@ -41,46 +39,6 @@ const MAX_DOWNLOAD_BYTES = 200 * 1024 * 1024;
 const DOWNLOAD_LOCK_TIMEOUT_MS = 120_000;
 const DOWNLOAD_LOCK_STALE_MS = 10 * 60_000;
 
-/**
- * Read the version string from an `aft` binary by invoking it with
- * `--version`. Returns the bare version (e.g. `"0.22.1"`) without the
- * leading `v` or the `aft` prefix, or `null` if the invocation fails.
- *
- * Shared by the downloader and resolver so both cache hot-paths validate the
- * binary itself instead of trusting directory names.
- */
-export function readBinaryVersion(binaryPath: string): string | null {
-  try {
-    const result = spawnSync(binaryPath, ["--version"], {
-      encoding: "utf-8",
-      stdio: ["pipe", "pipe", "pipe"],
-      timeout: 5000,
-    });
-    const stdoutVersion = result.stdout?.trim();
-    const stderrVersion = result.stderr?.trim();
-    const rawVersion = stdoutVersion || stderrVersion;
-    if (!rawVersion) return null;
-    // `aft --version` outputs "aft 0.9.0" — extract just the version number
-    return rawVersion.replace(/^aft\s+/, "");
-  } catch {
-    return null;
-  }
-}
-
-function expectedVersionFromTag(tag: string): string {
-  return tag.startsWith("v") ? tag.slice(1) : tag;
-}
-
-function isExpectedCachedBinary(binaryPath: string, tag: string): boolean {
-  const expected = expectedVersionFromTag(tag);
-  const actual = readBinaryVersion(binaryPath);
-  if (actual === expected) return true;
-  warn(
-    `Cached binary at ${binaryPath} reports ${actual ?? "no version"}, expected ${expected}; refreshing cache entry`,
-  );
-  return false;
-}
-
 /** Get the cache directory, respecting XDG_CACHE_HOME / LOCALAPPDATA. */
 export function getCacheDir(): string {
   if (process.platform === "win32") {
@@ -148,10 +106,8 @@ export async function downloadBinary(version?: string): Promise<string | null> {
   const binaryName = getBinaryName();
   const binaryPath = join(versionedCacheDir, binaryName);
 
-  // Already cached for this version. Probe the binary itself before trusting
-  // the cache directory name; stale hot-swap entries can otherwise shadow a
-  // freshly requested compatible version forever.
-  if (existsSync(binaryPath) && isExpectedCachedBinary(binaryPath, tag)) {
+  // Already cached for this version
+  if (existsSync(binaryPath)) {
     return binaryPath;
   }
 
@@ -177,9 +133,7 @@ export async function downloadBinary(version?: string): Promise<string | null> {
     releaseLock = await acquireDownloadLock(lockPath);
 
     // Another process may have completed the same version while we waited.
-    // Re-probe here too because a stale owner might have left a mismatched
-    // binary in the versioned directory before this process acquired the lock.
-    if (existsSync(binaryPath) && isExpectedCachedBinary(binaryPath, tag)) {
+    if (existsSync(binaryPath)) {
       return binaryPath;
     }
 
@@ -264,22 +218,23 @@ export async function downloadBinary(version?: string): Promise<string | null> {
     }
     log(`Checksum verified (SHA-256: ${actualHash.slice(0, 16)}...)`);
 
-    // Make executable
-    if (process.platform !== "win32") {
+    // Atomic rename (POSIX) or copy (Windows — renameSync fails with EEXIST
+    // when target exists). On Windows, copyFileSync overwrites the target;
+    // if it fails the original binary at binaryPath is preserved.
+    if (process.platform === "win32") {
+      copyFileSync(tmpPath, binaryPath);
+    } else {
       chmodSync(tmpPath, 0o755);
+      renameSync(tmpPath, binaryPath);
     }
 
-    // Replace binary — on Windows renameSync fails (EEXIST) when the target
-    // exists, so unlink first. This creates a brief window where no binary
-    // exists at binaryPath — callers should handle a missing binary gracefully.
-    if (process.platform === "win32" && existsSync(binaryPath)) {
-      try {
-        unlinkSync(binaryPath);
-      } catch {
-        // best-effort; renameSync will surface the error if unlink fails
-      }
+    // Binary was replaced successfully. Clean up the temp file best-effort;
+    // a cleanup failure should NOT propagate as a download failure.
+    try {
+      if (existsSync(tmpPath)) unlinkSync(tmpPath);
+    } catch {
+      warn(`Could not clean up temporary download file ${tmpPath} — it can be removed manually.`);
     }
-    renameSync(tmpPath, binaryPath);
 
     log(`AFT binary ready at ${binaryPath}`);
     return binaryPath;
@@ -329,7 +284,7 @@ export async function ensureBinary(version?: string): Promise<string | null> {
     // Do NOT fall back to legacy flat cache — it may contain a different version,
     // causing an infinite spawn-check-replace loop.
     const versionCached = getCachedBinaryPath(tag);
-    if (versionCached && isExpectedCachedBinary(versionCached, tag)) {
+    if (versionCached) {
       log(`Found cached binary for ${tag}: ${versionCached}`);
       return versionCached;
     }
@@ -345,21 +300,17 @@ async function acquireDownloadLock(lockPath: string): Promise<() => void> {
   const startedAt = Date.now();
   while (true) {
     try {
-      const owner = `${process.pid}:${Date.now()}:${randomUUID()}`;
       const fd = openSync(lockPath, "wx");
-      writeSync(fd, owner);
       return () => {
         try {
           closeSync(fd);
         } catch {
           // already closed — ignore
         }
         try {
-          if (readFileSync(lockPath, "utf-8") === owner) {
-            rmSync(lockPath, { force: true });
-          }
+          rmSync(lockPath, { force: true });
         } catch {
-          // best-effort lock cleanup; missing or reclaimed locks are fine
+          // best-effort lock cleanup
         }
       };
     } catch (err) {
@@ -421,7 +372,3 @@ async function fetchLatestTag(): Promise<string | null> {
     clearTimeout(timeout);
   }
 }
-
-export const __test__ = {
-  acquireDownloadLock,
-};

packages/aft-bridge/src/onnx-runtime.ts

@@ -486,8 +486,10 @@ function findSystemOnnxRuntime(libName?: string): string | null {
               join(nugetPackageDir, entry.name, "runtimes", "win-arm64", "native"),
             );
           }
-        } catch {
-          // best-effort scan
+        } catch (err) {
+          warn(
+            `Failed to scan NuGet ONNX Runtime cache ${nugetPackageDir}: ${err instanceof Error ? err.message : String(err)}`,
+          );
         }
         return nugetPaths;
       })(),
@@ -522,8 +524,10 @@ function findSystemOnnxRuntime(libName?: string): string | null {
     // are less common on Windows and we want PATH discovery to succeed.
     let skipVersionCheck = false;
     if (process.platform === "win32") {
-      // Only do version check for common install paths, not arbitrary PATH entries
-      const isCommonPath = dir.includes("Program Files") || dir.includes("onnxruntime");
+      // Only do version check for common install paths, not arbitrary PATH entries.
+      // Windows paths are case-insensitive, so normalize to lower case for comparison.
+      const dirLower = dir.toLowerCase();
+      const isCommonPath = dirLower.includes("program files") || dirLower.includes("onnxruntime");
       if (!isCommonPath) skipVersionCheck = true;
     }
 

packages/aft-cli/src/lib/diagnostics.ts

@@ -1,4 +1,4 @@
-import { closeSync, existsSync, mkdirSync, openSync, readSync, statSync } from "node:fs";
+import { accessSync, closeSync, constants, existsSync, mkdirSync, openSync, readSync, statSync } from "node:fs";
 import type { HarnessAdapter } from "../adapters/types.js";
 import { type BinaryCacheInfo, getBinaryCacheInfo } from "./binary-cache.js";
 import { probeBinaryVersion } from "./binary-probe.js";
@@ -113,17 +113,18 @@ async function diagnoseHarness(adapter: HarnessAdapter): Promise<HarnessDiagnost
   const logPath = adapter.getLogFile();
   const pluginCache = adapter.getPluginCacheInfo();
 
-  // Ensure the storage directory exists so diagnostics report useful info
-  // instead of "(not created)" on fresh installs. The storage directory is
-  // normally created lazily by the bridge on first tool call, but the doctor
-  // command is a read-only diagnostic that should still display it.
-  if (!existsSync(storage)) {
+  // Check if the storage directory exists and is accessible. We do NOT create
+  // it here — the doctor command is a diagnostic tool and should not mutate
+  // filesystem state. The bridge creates it lazily on first tool call.
+  const storageAccessible = (() => {
+    if (!existsSync(storage)) return false;
     try {
-      mkdirSync(storage, { recursive: true });
+      accessSync(storage, constants.R_OK | constants.W_OK);
+      return true;
     } catch {
-      // best-effort — diagnostics can still report the path as (not created)
+      return false;
     }
-  }
+  })();
 
   const describeStorage =
     "describeStorageSubtrees" in adapter &&

packages/aft-cli/src/lib/onnx.ts

@@ -82,17 +82,6 @@ export function findSystemOnnxRuntime(): string | null {
       join(programFiles, "Microsoft Machine Learning", "lib"),
       join(programFilesX86, "onnxruntime", "lib"),
     );
-    // Also include absolute PATH entries. Use the same filters that the bridge
-    // version's pathEntriesForPlatform() applies: absolute-path check, null-byte
-    // rejection, "." exclusion, quote-stripping.
-    const delimiter = ";";
-    const pathEnv = process.env.PATH ?? "";
-    for (const dir of pathEnv.split(delimiter)) {
-      const trimmed = dir.trim();
-      if (!trimmed || trimmed === "." || trimmed.includes("\0")) continue;
-      if (!isAbsolute(trimmed)) continue;
-      searchPaths.push(trimmed);
-    }
   }
 
   // Deduplicate paths.