chore: post-review fixes for PR #66

[Zireael]

Summary

6 small fixes discovered during code review of PR #66, addressing edge cases and bugs across Rust and TypeScript layers.

Changes

1. semantic_index.rs: Buffer size 260→32767 for GetModuleFileNameW to prevent silent truncation on deep Windows paths (e.g., long NuGet package paths under %USERPROFILE%)
2. semantic_index.rs: Removed unreachable #[cfg(target_os = "windows")] dead code in suggest_removal_command (Windows paths never start with /usr/local/lib)
3. onnx.ts (CLI): Removed duplicate PATH scanning loop — pathEntriesForPlatform() already reads PATH with proper filtering
4. diagnostics.ts (CLI): Replaced mkdirSync side effect with read-only accessSync check — doctor should not mutate filesystem
5. onnx-runtime.ts (bridge): Normalized Windows path comparisons to lowercase for case-insensitive matching
6. onnx-runtime.ts (bridge): Added warning logging for NuGet scan failures

Verification

• tsc --noEmit clean in both aft-bridge and aft-cli
• cargo check + cargo clippy -D warnings pending (Docker unavailable on this machine — changes are trivially safe: buffer size bump + dead code removal)

Supersedes: PR #66

---

^{Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.}

---

Summary by cubic

Post-review hardening for PR #66: improves ONNX Runtime discovery/validation on Windows, makes Windows binary replacement safer and resilient to temp cleanup errors, and keeps diagnostics read-only. Also ensures storage is created during configure. Fixes CI flakes and Windows edge cases across Rust and TypeScript.

• Bug Fixes
  • Windows: validate onnxruntime.dll early via LoadLibraryExW, probe version with GetFileVersionInfoW, and align version gate with macOS/Linux (crates/aft/src/semantic_index.rs).
  • Prevent path truncation on deep NuGet paths by increasing GetModuleFileNameW buffer to 32767 (crates/aft/src/semantic_index.rs).
  • Improve Windows discovery: scan NuGet and common Program Files locations, log warnings on NuGet scan failures, and use case-insensitive path checks; remove duplicate PATH scanning in CLI (packages/aft-bridge/src/onnx-runtime.ts, packages/aft-cli/src/lib/onnx.ts).
  • Windows binary replacement: use copyFileSync(tmp, target) then best‑effort unlinkSync(tmp) so cleanup failures don’t fail the download; keep POSIX flow as chmodSync + atomic renameSync (packages/aft-bridge/src/downloader.ts).
  • Configure: create storage root with fs::create_dir_all and warn on failure (crates/aft/src/commands/configure.rs).
  • Doctor: replace mkdirSync side effect with a read-only accessSync check; no filesystem mutations (packages/aft-cli/src/lib/diagnostics.ts).
  • Cleanup: remove unreachable Windows branch in removal hint (crates/aft/src/semantic_index.rs).

^{Written for commit 663851b. Summary will update on new commits.}

Review in cubic

[cubic-dev-ai]

1 issue found across 7 files

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="packages/aft-bridge/src/downloader.ts">

<violation number="1" location="packages/aft-bridge/src/downloader.ts:275">
P2: Windows unlink-first strategy can permanently remove the existing binary if renameSync fails after successful unlink</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Zireael]

Addressed cubic review finding #1 (the unlink-then-rename pattern in downloader.ts):

Replaced unlinkSync + renameSync with copyFileSync + unlinkSync on Windows. If copyFileSync fails, the old binary at binaryPath is preserved — no more window where neither old nor new binary exists. On POSIX, renameSync remains the fast atomic path.

Verified tsc --noEmit clean in both aft-bridge and aft-cli.

Branch rebased cleanly onto latest cortexkit/main (which already includes the merged PR #66).

[cubic-dev-ai]

1 issue found across 5 files

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="packages/aft-bridge/src/downloader.ts">

<violation number="1" location="packages/aft-bridge/src/downloader.ts:275">
P2: Windows temp file cleanup failure can falsely report a successful binary replacement as a download failure</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[Zireael]

Addressed cubic finding P2 (Windows temp file cleanup falsely reporting download failure):

• Separated binary replacement (copyFileSync) from temp file cleanup (unlinkSync)
• If copyFileSync fails, the original binary at binaryPath is preserved and the outer catch correctly reports a download failure
• If copyFileSync succeeds but unlinkSync(tmpPath) fails, only a warning is logged — the download is reported as successful because the binary was correctly placed
• On POSIX, chmodSync + renameSync remains the fast atomic path

Verified tsc --noEmit clean on the updated branch.

[cubic-dev-ai]

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="packages/aft-bridge/src/downloader.ts">

<violation number="1" location="packages/aft-bridge/src/downloader.ts:110">
P2: Cache-hit validation was weakened from verifying the binary's runtime version to trusting path existence alone. Without `isExpectedCachedBinary`, a stale, corrupted, or manually replaced binary in the versioned cache directory will be silently reused.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: Cache-hit validation was weakened from verifying the binary's runtime version to trusting path existence alone. Without isExpectedCachedBinary, a stale, corrupted, or manually replaced binary in the versioned cache directory will be silently reused.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/aft-bridge/src/downloader.ts, line 110:

<comment>Cache-hit validation was weakened from verifying the binary's runtime version to trusting path existence alone. Without `isExpectedCachedBinary`, a stale, corrupted, or manually replaced binary in the versioned cache directory will be silently reused.</comment>

<file context>
@@ -149,10 +106,8 @@ export async function downloadBinary(version?: string): Promise<string | null> {
-  // freshly requested compatible version forever.
-  if (existsSync(binaryPath) && isExpectedCachedBinary(binaryPath, tag)) {
+  // Already cached for this version
+  if (existsSync(binaryPath)) {
     return binaryPath;
   }
</file context>