Skip to content

fix(model): discover Windows npm CLI installs (BRNS-DESK-069) - #92

Merged
stefan-ssv-labs merged 2 commits into
devfrom
fix/desk-069-windows-cli-discovery-clean
Aug 14, 2026
Merged

fix(model): discover Windows npm CLI installs (BRNS-DESK-069)#92
stefan-ssv-labs merged 2 commits into
devfrom
fix/desk-069-windows-cli-discovery-clean

Conversation

@stefan-ssv-labs

@stefan-ssv-labs stefan-ssv-labs commented Aug 14, 2026

Copy link
Copy Markdown

Summary

Fixes the remaining Windows CLI-discovery gap for GUI-launched desktop sessions with a stale inherited PATH.

Ticket: BRNS-DESK-069

Requirements → changes

  • Add deterministic, user-local Windows candidates from dirs Known Folder APIs: roaming npm, local npm, the default Codex installer bin, and ~/.codex/bin.
  • Keep the existing shared augmented-path seam used by Claude and Codex; explicit binary overrides, cache behavior, suffix order, existence filtering, and Unix candidate order remain unchanged.
  • Add a Windows-gated unit test with temporary .cmd shims proving both claude and codex resolve when inherited PATH is empty.
  • Retain known-folder candidates even when the profile-home lookup is unavailable; no relative path is formed from a missing root.

Regression provenance

  • Parity plan PR #47 and its required rebase review narrowed this distinct directory-discovery remainder.
  • PR #68 is legacy-main location prior art, not duplicated code.
  • The separately landed Windows PATH-separator fix remains out of scope.

Reproduction / current gap

Source-derived; no Windows hardware run was performed on this macOS host.

  1. Install a provider CLI into %APPDATA%\\npm or %LOCALAPPDATA%\\npm, or use the default Codex installer bin.
  2. Start the desktop app from a Windows GUI session whose inherited PATH predates that installation, with no explicit binary override.
  3. Before this change, shared discovery depended on inherited PATH because it did not prepend those deterministic locations.

Validation

Passed on the rebased branch:

  • cargo fmt --all --check
  • npm run lint:size
  • cargo test -p brains-model — 182 passed, 2 ignored
  • cargo check -p brains-model --tests --target x86_64-pc-windows-msvc
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo check -p brains-browser --features chromium (three existing browser warnings only)

cargo test --workspace ran through the affected model and desktop suites, but remains non-green on this macOS host because two headless_sandbox integration tests cannot access local Keychain services (SecKeychainCopySearchList / KEYCHAIN-DENIED). The exact two failures reproduce unchanged in a clean detached worktree at unmodified origin/dev 26d5c7694ede7aadf326f1f54b0d433a7d072285; they are not introduced by this change. CI remains the actual Windows execution lane.

Risk, scope, and rollout

  • No package, capability, IPC, persistence, network, shell, or installer changes.
  • The augmented path remains cached; this adds only four fixed candidate directories to existing existence checks.
  • CODEX_INSTALL_DIR and arbitrary npm prefixes are deliberately deferred; inherited PATH and explicit binary overrides still cover custom installations.
  • A full Windows GUI/hardware run is intentionally owned by downstream BRNS-DESK-048.

Merge method: squash (do not merge-commit or rebase-merge).

@nir-ssvlabs nir-ssvlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property worth proving here is that no candidate can come out relative, since these are prepended to PATH — a .codex/bin formed from an absent root would resolve against the CWD and let anything sitting there shadow the real CLI. It's structurally impossible as written: every candidate is built inside a map/if let on its Option<&Path>, the Nones are dropped by flatten, and the home-relative entries sit behind let Some(home) = home else { return dirs; }. That early return is also what delivers the "keep known-folder candidates when the profile home is unavailable" requirement rather than falling back to an empty list.

Unix is genuinely untouched: #[cfg(not(windows))] starts from an empty vec and the same .extend([...]) list follows in the same order, so both the populated and the home == None cases produce exactly what they did before. The dirs mapping is right too — data_dir() is %APPDATA% and data_local_dir() is %LOCALAPPDATA% on Windows, which is what the roaming/local npm paths need.

Checked: candidate construction against a missing root, the prepend order and that explicit binary overrides are untouched, the Unix path's before/after equivalence including the None home case, and the Known Folder mapping. Not read: nothing material — this is small and the risk is concentrated in one function.

Merge: ✅ into dev once Rust (Windows) lands green — it was still pending, and since no Windows hardware run was possible, that job is the only proof the new gated test actually passes.

@stefan-ssv-labs stefan-ssv-labs left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ review-pr (self-review, cannot approve own PR): clean — no blocker or critical issue found.\n\n

@stefan-ssv-labs
stefan-ssv-labs merged commit 92949c0 into dev Aug 14, 2026
6 of 7 checks passed
@stefan-ssv-labs
stefan-ssv-labs deleted the fix/desk-069-windows-cli-discovery-clean branch August 14, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants