diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 052ef724..a91ee873 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,7 +59,24 @@ jobs: strategy: fail-fast: false matrix: - os: [macos-latest] + # WINDOWS IS A SHIPPING PLATFORM, so it is built here. It was not, and the cost + # was exact: `browser_host.rs` reached for `ns_window()` — macOS-only — in a + # module `lib.rs` declares unconditionally, and the default Windows build stopped + # linking. Every job was green through review and merge, because not one of them + # compiled for Windows. `fail-fast: false` so a break on one OS still reports the + # other rather than hiding it. + # + # THIS MATRIX REPLACED A SEPARATE `Windows (recording)` job (#45), which existed + # only because extending the matrix was impossible before this PR: `--workspace` + # was red on Windows (`agents/local/src/sandbox.rs` asserted `/private/tmp` + # ungated) and `brains-desktop` did not link there at all. Both are fixed here, so + # the matrix now covers strictly more than that job did — its clippy and tests are + # the two steps below — and two Windows mechanisms would just drift apart. + # + # NEVER `continue-on-error` on either leg. A job that cannot go red is worse than + # no job, because it reads as coverage: a green Windows job in the reference app + # once hid 14 failing tests. + os: [macos-latest, windows-latest] steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 @@ -79,14 +96,20 @@ jobs: with: workspaces: src-tauri + # `shell: bash` on every scripted step from here down. The Windows runner defaults + # to PowerShell, where `if ! cmd; then`, `mkdir -p` and `: >` are syntax errors — + # and GitHub ships Git Bash on that image, so one line keeps a single script for + # both operating systems instead of a PowerShell fork of each. + # # NOT WIDENED TO `cargo fmt --all --check`, though it should be. The - # engine crates are unchecked today, and widening was scoped into this - # PR — but the base is ~160 fmt-dirty sites across the workspace, and + # engine crates are unchecked today, and widening was scoped into #45 + # — but the base is ~160 fmt-dirty sites across the workspace, and # this narrower check is ALREADY failing on dev (src-tauri/src/lib.rs # alone has 11). Widening it here would mean either landing red or - # burying an audio review under a whole-workspace reformat. Do the + # burying a review under a whole-workspace reformat. Do the # reformat as its own commit, then add --all. - name: Format check + shell: bash run: | if ! cargo fmt --manifest-path src-tauri/Cargo.toml --check; then echo "" @@ -96,6 +119,7 @@ jobs: fi - name: Create frontend build stub + shell: bash run: mkdir -p build # A bare-cargo job runs no beforeBuildCommand, and tauri_build hard-fails @@ -124,6 +148,12 @@ jobs: run: cargo test --workspace - name: Check the bundled Chromium engine compiles + # macOS ONLY, and not an oversight: the engine's own modules are AppKit — `app_mac`, + # `clip`, and the `objc2-app-kit` views `engine.rs` is written against. The feature + # does not claim to build off macOS (`platform_supported()` answers for Windows via + # the OS webview), so asking Windows to type-check it would fail on the honest + # absence of a macOS-only backend rather than on anything wrong. + if: runner.os == 'macOS' # ~2,000 feature-gated lines are invisible to every step above, because the engine # is off by default and CI has no reason to turn it on. Without this a change to # `Layout` or `host::Host` breaks them silently and the break surfaces on a @@ -133,59 +163,6 @@ jobs: # it type-checks the code against the pinned bindings, which is the part that rots. run: cargo check -p brains-browser --features chromium - # WHY THIS JOB EXISTS: call recording had never been run on Windows, and the - # reason it could go that long is that nothing here ever compiled for it. - # The bugs that cost a bring-up day were a verbatim resource path, a missing - # process-tree kill, and a wire type in snake_case — the first two are - # Windows-only by construction and the third was invisible everywhere. - # - # It is deliberately NARROW and deliberately BLOCKING. Never - # continue-on-error: a job that cannot go red is worse than no job, because - # it reads as coverage — a green Windows job here once hid 14 failing tests. - windows: - name: Windows (recording) - runs-on: windows-latest - timeout-minutes: 45 - defaults: - run: - # Every step here is POSIX; the runner's default shell is pwsh. - shell: bash - steps: - - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 - - - uses: dtolnay/rust-toolchain@4cda84d5c5c54efe2404f9d843567869ab1699d4 # stable branch - with: - toolchain: stable - components: clippy - - - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 - with: - workspaces: src-tauri - - # SCOPED TO brains-recording ON PURPOSE, and the scope is the whole - # design of this job. Two things are knowingly excluded: - # - # * --workspace, because src/engines/agents/local/src/sandbox.rs - # asserts real_path("/tmp") == "/private/tmp" with no platform gate, - # so the workspace is red on Windows for reasons that have nothing - # to do with recording. - # * -p brains-desktop --lib, because it pulls brains-storage in and - # that crate has its own pre-existing clippy debt (backups.rs - # sort_by_key, previews.rs needless borrows). - # - # Both are worth fixing and both are their own PR. Widening this job - # before they are fixed would mean landing it red, and a red-on-arrival - # job gets `continue-on-error` bolted on within a week — which is the - # exact failure this job exists to avoid. - - name: Clippy (recording engine, tests included) - run: cargo clippy -p brains-recording --all-targets -- -D warnings - - - name: Unit tests (recording engine) - run: cargo test -p brains-recording - - - name: Build the recording engine - run: cargo build -p brains-recording - eval: name: Eval (token-less) runs-on: macos-latest diff --git a/.gitignore b/.gitignore index 65354fc2..4cbbad7f 100644 --- a/.gitignore +++ b/.gitignore @@ -35,6 +35,11 @@ src-tauri/resources/ # committed: hundreds of MB of vendor native code, and the bundled Node. src-tauri/recall/ src-tauri/binaries/ +# The verified SDK archive `package-runtime.mjs` builds, plus its manifest — +# ~37MB, generated, and the one staging directory the list above missed. It was +# merely untracked, which is not the same as ignored: it sat in every +# `git status` waiting for one `git add -A` to commit the vendored SDK. +src-tauri/recall-runtime/ # Environment .env diff --git a/scripts/dev/recall/package-runtime.mjs b/scripts/dev/recall/package-runtime.mjs index 2f247a6f..dc4c997b 100644 --- a/scripts/dev/recall/package-runtime.mjs +++ b/scripts/dev/recall/package-runtime.mjs @@ -77,8 +77,12 @@ if (!existsSync(join(sdkDir, "node_modules"))) { ]); } // Through host.mjs: GNU tar (first on PATH under Git for Windows) reads the -// `C:` in an absolute path as a hostname and aborts, so every path goes -// relative to an explicit cwd. A no-op on macOS and Linux. +// `C:` in an absolute path as a hostname and aborts ("Cannot connect to C: +// resolve failed"), so every path goes relative to an explicit cwd. A no-op on +// macOS and Linux, where bsdtar accepts either form. +// +// This runs inside `beforeBuildCommand`, so getting it wrong fails the whole +// build rather than just the recording assets. tar(root, ["-czf", temporaryArchive, "-C", dirname(sdkDir), basename(sdkDir)]); await rm(archivePath, { force: true }); await rename(temporaryArchive, archivePath); diff --git a/src-tauri/src/browser_host.rs b/src-tauri/src/browser_host.rs index 38c4fa7b..ba2e2dbb 100644 --- a/src-tauri/src/browser_host.rs +++ b/src-tauri/src/browser_host.rs @@ -65,8 +65,23 @@ impl Host for TauriHost { // `None` for a window that is not there is a NORMAL answer, not an error: a view // can be asked for while the window is being torn down, and the engine treats // absence as "nothing to parent into" rather than a failure. + // + // WHICH IS ALSO WHY THE NON-macOS ARM IS `None` RATHER THAN A STUB THAT PANICS. + // `ns_window()` exists only on macOS, and this file is compiled on every + // platform because `lib.rs` declares the module unconditionally — so without the + // gate the DEFAULT Windows build does not compile at all, which is what shipped. + // There is no NSWindow off macOS, "nothing to parent into" is the truth there, + // and the trait already says that is an ordinary answer. let window = self.app.get_webview_window(window_label)?; - window.ns_window().ok().map(WindowPtr) + #[cfg(target_os = "macos")] + { + window.ns_window().ok().map(WindowPtr) + } + #[cfg(not(target_os = "macos"))] + { + let _ = window; + None + } } fn emit_report(&self, key: &str, payload: &str) { diff --git a/src/engines/agents/local/src/actor_runner.rs b/src/engines/agents/local/src/actor_runner.rs index e18c252d..714337b0 100644 --- a/src/engines/agents/local/src/actor_runner.rs +++ b/src/engines/agents/local/src/actor_runner.rs @@ -268,6 +268,8 @@ fn mark_terminal(storage: &Storage, run_id: &str, status: StoredStatus, detail: #[cfg(test)] pub(crate) mod tests { use super::*; + // Only `runner` below takes one, and that is macOS-gated with it. + #[cfg(target_os = "macos")] use std::path::Path; pub(crate) fn spec() -> AgentRunSpec { @@ -350,6 +352,9 @@ pub(crate) mod tests { Arc::new(move |_app_id| Ok(workspace.clone())) } + // Its only caller is the macOS-gated test below; `runner_settings` builds its + // own with a dynamic workspace resolver. + #[cfg(target_os = "macos")] pub(crate) fn runner(tmp: &Path) -> (Arc, ActorRunner) { let storage = Arc::new(Storage::init_at(tmp.join("data-root")).unwrap()); let workspace = tmp.join("workspace"); @@ -368,6 +373,7 @@ pub(crate) mod tests { (storage, runner) } + #[cfg(target_os = "macos")] // elsewhere `policy` is `Unsupported`: refused before a run exists #[tokio::test] async fn a_run_that_cannot_spawn_reports_a_failure_and_still_persists_a_run() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/engines/agents/local/src/runner_settings.rs b/src/engines/agents/local/src/runner_settings.rs index f4cac460..17bdf1e3 100644 --- a/src/engines/agents/local/src/runner_settings.rs +++ b/src/engines/agents/local/src/runner_settings.rs @@ -87,8 +87,13 @@ impl ActorRunner { #[cfg(test)] mod tests { use super::*; - use crate::actor_runner::tests::{no_credential, spec}; + use crate::actor_runner::tests::no_credential; + // `spec` and `RunnerPort` serve the macOS-gated tail below and nothing + // else: `spawn_plan` is inherent, so only `spawn_headless` needs the trait. + #[cfg(target_os = "macos")] + use crate::actor_runner::tests::spec; use crate::actor_runner::WorkspaceResolver; + #[cfg(target_os = "macos")] use crate::runner::RunnerPort; use std::sync::{Arc, Mutex}; @@ -145,7 +150,7 @@ mod tests { #[tokio::test] async fn a_settings_change_reaches_the_next_scheduled_spawn() { let tmp = tempfile::tempdir().unwrap(); - let (storage, runner) = runner_with_dynamic_workspace(tmp.path()); + let (_storage, runner) = runner_with_dynamic_workspace(tmp.path()); let before = runner.spawn_plan(&ALLOWED, "sample").unwrap(); assert_eq!(before.cwd, tmp.path().join("workspace")); @@ -189,14 +194,24 @@ mod tests { // And the run the scheduler actually fires uses them: the run on disk // is stamped with the cwd from the workspace resolver. - let handle = runner.spawn_headless(spec()).expect("the run was created"); - let run_id = handle.run_id.clone(); - let _ = handle.wait().await; - // The cwd comes from the workspace resolver, not directly from settings. - // In a full integration, the resolver reads from settings, so the effect - // is the same. - let meta = storage.read_run_meta(&run_id).unwrap(); - // Just verify the run completed and has a cwd set - assert!(!meta.cwd.as_os_str().is_empty()); + // + // THIS LAST STEP ONLY is macOS-gated, and the split is deliberate: the + // live-settings claim above is what this test is for, and it holds on + // every platform. `spawn_headless` is what does not — off macOS it + // refuses before a run exists, because there is no seatbelt to confine + // one to (`sandbox::policy` answers `Unsupported`), so there would be + // no run on disk to read a cwd off. Gating the whole test instead would + // have thrown away the portable half to buy nothing. + #[cfg(target_os = "macos")] + { + let handle = runner.spawn_headless(spec()).expect("the run was created"); + let run_id = handle.run_id.clone(); + let _ = handle.wait().await; + // The cwd comes from the workspace resolver, not directly from + // settings. In a full integration, the resolver reads from + // settings, so the effect is the same. + let meta = _storage.read_run_meta(&run_id).unwrap(); + assert!(!meta.cwd.as_os_str().is_empty()); + } } } diff --git a/src/engines/agents/local/src/sandbox.rs b/src/engines/agents/local/src/sandbox.rs index bd7d5b70..1c764506 100644 --- a/src/engines/agents/local/src/sandbox.rs +++ b/src/engines/agents/local/src/sandbox.rs @@ -460,6 +460,11 @@ mod tests { /// Seatbelt matches RESOLVED paths, so a root handed over unresolved /// grants nothing at all — silently. `/tmp` is a symlink on macOS, which /// makes it the cheapest way to prove the resolution really happens. + /// + /// …and is also why this is macOS-only: the whole check is built on that + /// symlink. `/tmp` resolving to `/private/tmp` is a fact about macOS, not + /// about the resolver, and off macOS there is no `/tmp` to resolve. + #[cfg(target_os = "macos")] #[test] fn roots_reach_the_profile_as_real_paths() { let resolved = real_path(Path::new("/tmp")).expect("/tmp exists"); @@ -495,6 +500,12 @@ mod tests { /// A write root that cannot be resolved is an ERROR, not a silently /// dropped rule — a `(subpath "/nope")` would have granted nothing and /// said so to nobody, and the run would have failed somewhere far away. + /// + /// macOS-only because `policy()` checks the PLATFORM before it checks the + /// roots: off macOS every call is `Err(Unsupported)`, so the test would + /// pass for the wrong reason — it would never reach the resolution it + /// exists to pin. + #[cfg(target_os = "macos")] #[test] fn an_unresolvable_root_refuses_to_produce_a_policy() { let roots = SandboxRoots::for_run("/no-such-volume-xyz/run", "/tmp"); diff --git a/src/engines/agents/local/tests/headless_sandbox.rs b/src/engines/agents/local/tests/headless_sandbox.rs index e723a681..3696cc79 100644 --- a/src/engines/agents/local/tests/headless_sandbox.rs +++ b/src/engines/agents/local/tests/headless_sandbox.rs @@ -33,23 +33,45 @@ //! another shape of "the boundary holds", and these tests share fixtures that //! cannot be split without duplicating the ENV_LOCK/Env machinery. +// EVERY TEST HERE IS macOS-ONLY BUT ONE, and so is everything they lean on — +// which is why the gates run down the imports and helpers as well as the tests. +// A `#[cfg]` on the test alone leaves its fixtures compiling on a platform that +// cannot host them: `probing_cli` writes a `#!/bin/sh` and chmods it through +// `std::os::unix`, which does not exist off Unix, so the whole test binary +// failed to build on Windows and took `cargo test --workspace` down with it. +// +// The one survivor reads `sandbox::PROFILE` as a string, which is why it is the +// survivor: everything else here reaches `sandbox::policy()`, and off macOS that +// is `Err(Unsupported)` by design — there is no seatbelt to ask. Keeping the +// profile check portable is deliberate rather than incidental; the absence it +// asserts (no `mach-lookup` grant) is the one a careless edit reopens, and it +// should fail on whichever machine the careless edit was made. +#[cfg(target_os = "macos")] use std::path::{Path, PathBuf}; +#[cfg(target_os = "macos")] use std::sync::{Arc, Mutex, MutexGuard}; +#[cfg(target_os = "macos")] use brains_local_agents::actor_runner::ActorRunner; +#[cfg(target_os = "macos")] use brains_local_agents::runner::{AgentRunSpec, RunStatus, RunnerPort}; use brains_local_agents::sandbox; +#[cfg(target_os = "macos")] use brains_model::session_actor::{EventSink, SessionRegistry}; +#[cfg(target_os = "macos")] use brains_storage::{RunStatus as StoredStatus, Storage}; +#[cfg(target_os = "macos")] static ENV_LOCK: Mutex<()> = Mutex::new(()); /// Sets environment variables for the length of the test and restores them. +#[cfg(target_os = "macos")] struct Env { _lock: MutexGuard<'static, ()>, restore: Vec<(String, Option)>, } +#[cfg(target_os = "macos")] impl Env { fn new() -> Self { Self { @@ -69,6 +91,7 @@ impl Env { } } +#[cfg(target_os = "macos")] impl Drop for Env { fn drop(&mut self) { for (key, previous) in self.restore.iter().rev() { @@ -82,6 +105,7 @@ impl Drop for Env { /// The declared job a tick would build. Values do not matter here — this file /// is about what the CHILD may do, not about what the prompt says. +#[cfg(target_os = "macos")] fn spec() -> AgentRunSpec { AgentRunSpec { run_uuid: "morning:2026-08-05".into(), @@ -103,6 +127,7 @@ fn spec() -> AgentRunSpec { /// A stand-in for the CLI that PROBES ITS OWN CONFINEMENT before answering: /// `probe` is shell run in the child, inside whatever boundary the engine put /// it in, and its findings are the evidence these tests read. +#[cfg(target_os = "macos")] fn probing_cli(dir: &Path, probe: &str) -> PathBuf { use std::os::unix::fs::PermissionsExt; let path = dir.join("fake-claude"); @@ -123,6 +148,7 @@ printf '%s\n' '{{"type":"result","subtype":"success","is_error":false,"duration_ path } +#[cfg(target_os = "macos")] fn runner(tmp: &Path, workspace: &Path, bin: PathBuf) -> (Arc, ActorRunner) { let storage = Arc::new(Storage::init_at(tmp.join(".brains")).unwrap()); let workspace_path = workspace.to_path_buf(); @@ -304,6 +330,7 @@ if /usr/bin/security list-keychains >/dev/null 2>&1; then echo KEYCHAIN-ALLOWED // add ways for them to fail for other reasons. /// A run's roots over a temp dir, with the two write roots created. +#[cfg(target_os = "macos")] fn prepared() -> (tempfile::TempDir, sandbox::SandboxRoots) { let tmp = tempfile::tempdir().unwrap(); std::fs::create_dir_all(tmp.path().join("run")).unwrap(); @@ -566,6 +593,7 @@ fn the_mach_allowlist_is_exactly_the_keychain_door() { /// The wrapper argv, as the child will really be exec'd: parameters, then /// the profile, then whatever is appended. +#[cfg(target_os = "macos")] #[test] fn the_wrapper_is_sandbox_exec_with_the_profile_inline() { let (_tmp, roots) = prepared(); diff --git a/src/engines/brains/native/src/keychain.rs b/src/engines/brains/native/src/keychain.rs index 0f8f3715..1789c82d 100644 --- a/src/engines/brains/native/src/keychain.rs +++ b/src/engines/brains/native/src/keychain.rs @@ -30,6 +30,26 @@ fn entry(service: &str, account: &str) -> Result { Entry::new(service, account).map_err(BrainsError::from) } +/// Where the dev-token file lives, for the three debug paths that touch it. +/// +/// `BRAINS_HOME` first, for the reason it exists everywhere else. Then the user's home +/// — and that is TWO variables, not one: `HOME` is a Unix name, and on Windows it is +/// set only when something like Git Bash synthesises it. A GUI launch has just +/// `USERPROFILE`, so a `HOME`-only lookup resolved to nothing and quietly skipped the +/// dev-token entirely — working under `npm run dev` from a bash shell and not when the +/// same binary is double-clicked. Checking both is what makes this agree with +/// `brains_storage::default_root()`, which this module deliberately does not depend on. +#[cfg(debug_assertions)] +fn dev_root() -> Option { + if let Some(explicit) = std::env::var_os("BRAINS_HOME").filter(|v| !v.is_empty()) { + return Some(std::path::PathBuf::from(explicit)); + } + std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .filter(|v| !v.is_empty()) + .map(|home| std::path::PathBuf::from(home).join(".brains-dev")) +} + /// `Ok(None)` = no entry yet (first launch / after logout). pub fn get_token() -> Result, BrainsError> { // Dev builds: keychain ACLs are per-binary, and every `tauri dev` rebuild @@ -52,14 +72,7 @@ pub fn get_token() -> Result, BrainsError> { // have to: this block is `debug_assertions` only, so the only default // it can ever need is the DEBUG one (`brains_storage::root::DIR_NAME`, // `.brains-dev`). `BRAINS_HOME` is honoured for the same reason it is there. - let root = std::env::var_os("BRAINS_HOME") - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - std::env::var_os("HOME") - .map(|home| std::path::PathBuf::from(home).join(".brains-dev")) - }); - if let Some(root) = root { + if let Some(root) = dev_root() { if let Ok(tok) = std::fs::read_to_string(root.join("dev-token")) { let tok = tok.trim().to_string(); if tok.starts_with("brn_") { @@ -67,11 +80,11 @@ pub fn get_token() -> Result, BrainsError> { } } } - // AND STOP HERE. "Never wake the keychain dialog during iteration" is what the - // paragraph above promises, and falling through to the keychain broke that - // promise on any machine where neither dev source is set: the item's ACL names - // whichever binary last answered a dialog, every rebuild is a new binary, and so - // every launch raised one again. + // AND ON macOS, STOP HERE. "Never wake the keychain dialog during iteration" is + // what the paragraph above promises, and falling through to the keychain broke + // that promise on any machine where neither dev source is set: the item's ACL + // names whichever binary last answered a dialog, every rebuild is a new binary, + // and so every launch raised one again. // // That is not merely annoying. `assemble()` calls this during boot, on the main // thread, so the dialog blocks startup — and with the bundled Chromium engine @@ -80,6 +93,15 @@ pub fn get_token() -> Result, BrainsError> { // build that cannot boot until someone types a password is not. // // Release builds are unaffected: one stable signed binary, one Always Allow. + // + // macOS ONLY, and the gate is the whole point. The dialog this avoids is a + // macOS mechanism: keychain items carry a per-binary ACL there. Windows + // Credential Manager has no such ACL and never prompts — a read is silent — so + // off macOS this early return bought nothing and cost the only source that + // works. A Windows dev build with a token sitting in Credential Manager under + // this exact service/account reported "no brains token" on every launch, and + // stayed signed out of boards, mini-sites and the MCP tool list. + #[cfg(target_os = "macos")] return Ok(None); } #[allow(unreachable_code)] @@ -93,14 +115,7 @@ pub fn get_token() -> Result, BrainsError> { pub fn save_token(token: &str) -> Result<(), BrainsError> { #[cfg(debug_assertions)] { - let root = std::env::var_os("BRAINS_HOME") - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - std::env::var_os("HOME") - .map(|home| std::path::PathBuf::from(home).join(".brains-dev")) - }); - if let Some(root) = root { + if let Some(root) = dev_root() { // CREATED 0600, never chmod'd TO it. `write` then `set_permissions` leaves the // token world-readable for the window between the two — short, but a token is // exactly the thing worth not leaving on the floor even briefly. The mode is @@ -142,15 +157,8 @@ pub fn delete_token() -> Result<(), BrainsError> { // read path prefers that file and would hand the deleted token straight back. #[cfg(debug_assertions)] { - if let Some(home) = std::env::var_os("BRAINS_HOME") - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - std::env::var_os("HOME") - .map(|home| std::path::PathBuf::from(home).join(".brains-dev")) - }) - { - let _ = std::fs::remove_file(home.join("dev-token")); + if let Some(root) = dev_root() { + let _ = std::fs::remove_file(root.join("dev-token")); } } delete_secret(SERVICE, ACCOUNT) diff --git a/src/engines/model/src/claude/path.rs b/src/engines/model/src/claude/path.rs index 2471a6ac..b859a119 100644 --- a/src/engines/model/src/claude/path.rs +++ b/src/engines/model/src/claude/path.rs @@ -9,6 +9,23 @@ use std::process::{Command, Stdio}; use std::sync::{Mutex, OnceLock}; use std::time::Duration; +/// THE PATH LIST SEPARATOR — `:` on Unix, `;` on Windows, and not to be +/// confused with `MAIN_SEPARATOR`, which is the one INSIDE a path. +/// +/// Hardcoding `:` does not merely fail to split on Windows; it splits in the +/// WRONG PLACES, because every Windows entry carries a drive letter. +/// `C:\Windows\system32` came apart into `C` and `\Windows\system32` — neither +/// of them a directory — so the composed PATH was rubble, every entry of it was +/// dropped by the existence check or was nonsense, and `resolve_claude_path` +/// searched it for a CLI it could never find. The app then reported the CLI as +/// missing on a machine where it was installed and on PATH. +/// +/// NOT `std::env::join_paths`, which is the obvious alternative: it returns a +/// `Result` that fails whenever any single entry contains the separator, and +/// throwing away the WHOLE PATH because one entry is malformed is a worse +/// failure than the one being fixed. Splitting is equivalent either way. +pub(crate) const PATH_SEP: char = if cfg!(windows) { ';' } else { ':' }; + /// A login shell can print an rc banner before `$PATH`; the delimiters make /// the payload findable regardless. const DELIM: &str = "__BRAINS_PATH__"; @@ -93,13 +110,13 @@ fn compose_path(well_known: &[PathBuf], login_shell: Option<&str>, inherited: &s for dir in well_known { push(&dir.to_string_lossy(), true, &mut out, &mut seen); } - for entry in login_shell.unwrap_or_default().split(':') { + for entry in login_shell.unwrap_or_default().split(PATH_SEP) { push(entry, true, &mut out, &mut seen); } - for entry in inherited.split(':') { + for entry in inherited.split(PATH_SEP) { push(entry, false, &mut out, &mut seen); } - out.join(":") + out.join(&PATH_SEP.to_string()) } /// Where CLIs actually install on a developer machine. Order matters: the @@ -268,10 +285,11 @@ pub fn resolve_claude_path(override_bin: Option<&Path>) -> Result) -> PathDiagnostics { let augmented = augmented_path(); PathDiagnostics { - entry_count: augmented.split(':').count(), + entry_count: augmented.split(PATH_SEP).count(), augmented_path: augmented, login_shell: std::env::var("SHELL").ok(), claude_binary: resolve_claude_path(override_bin).ok(), @@ -363,12 +381,12 @@ mod tests { let real = touch_dir(tmp.path(), "real/bin"); let missing = tmp.path().join("nope/bin"); - let path = compose_path( - &[real.clone(), missing.clone()], - None, - "/inherited/only:/usr/bin", - ); - let entries: Vec<&str> = path.split(':').collect(); + // The inherited half is written in this platform's own PATH syntax — + // the point of the test is that a real inherited entry survives, and a + // Unix-spelled one on Windows would be testing the spelling instead. + let inherited = ["/inherited/only", "/usr/bin"].join(&PATH_SEP.to_string()); + let path = compose_path(&[real.clone(), missing.clone()], None, &inherited); + let entries: Vec<&str> = path.split(PATH_SEP).collect(); assert!(entries.contains(&real.to_str().unwrap())); assert!(!entries.contains(&missing.to_str().unwrap())); assert!( @@ -383,23 +401,50 @@ mod tests { let bin = touch_dir(tmp.path(), "bin"); let bin_str = bin.to_str().unwrap().to_string(); - let path = compose_path( - std::slice::from_ref(&bin), - Some(&bin_str), - &format!("{bin_str}:/usr/bin"), - ); - assert_eq!(path, format!("{bin_str}:/usr/bin")); + let inherited = [bin_str.as_str(), "/usr/bin"].join(&PATH_SEP.to_string()); + let path = compose_path(std::slice::from_ref(&bin), Some(&bin_str), &inherited); + assert_eq!(path, inherited); } #[test] fn login_shell_entries_are_existence_checked() { let tmp = tempfile::tempdir().unwrap(); let real = touch_dir(tmp.path(), "shell/bin"); - let login = format!("{}:{}", real.display(), tmp.path().join("ghost").display()); + let login = format!( + "{}{PATH_SEP}{}", + real.display(), + tmp.path().join("ghost").display() + ); let path = compose_path(&[], Some(&login), ""); assert_eq!(path, real.to_string_lossy()); } + /// THE SEPARATOR BUG, pinned where it actually did its damage: the entries + /// that get existence-checked. Two real directories go in and both must + /// come out. + /// + /// On Windows the old `:` cut every `C:\…` entry into a drive letter and a + /// remainder. Neither is a directory, so the check dropped all of them and + /// the composed PATH came back EMPTY — which is why the symptom was "the + /// CLI is not installed" on a machine where it was. A round-trip assertion + /// would not have caught it: splitting on `:` and re-joining on `:` + /// reassembles the same string, so only a check that resolves the entries + /// can tell the difference. + #[test] + fn real_directories_survive_the_platform_separator() { + let tmp = tempfile::tempdir().unwrap(); + let first = touch_dir(tmp.path(), "a/bin"); + let second = touch_dir(tmp.path(), "b/bin"); + let login = [first.to_str().unwrap(), second.to_str().unwrap()].join(&PATH_SEP.to_string()); + + let composed = compose_path(&[], Some(&login), ""); + assert_eq!( + composed.split(PATH_SEP).collect::>(), + vec![first.to_str().unwrap(), second.to_str().unwrap()], + "both real dirs must survive: {composed}" + ); + } + #[test] fn extract_delimited_survives_an_rc_banner() { let out = format!("welcome to your shell!\n{DELIM}/a/bin:/b/bin{DELIM}"); @@ -497,7 +542,7 @@ mod tests { let path = augmented_path(); println!( "augmented PATH ({} entries):\n{path}", - path.split(':').count() + path.split(PATH_SEP).count() ); let bin = resolve_claude_path(None).expect("claude CLI not found"); println!("resolved claude: {}", bin.display()); diff --git a/src/engines/model/src/codex/path.rs b/src/engines/model/src/codex/path.rs index 29b5bff6..23f89d51 100644 --- a/src/engines/model/src/codex/path.rs +++ b/src/engines/model/src/codex/path.rs @@ -8,7 +8,7 @@ use std::path::{Path, PathBuf}; use std::sync::{Mutex, OnceLock}; -use crate::claude::path::{augmented_path, find_in_dirs}; +use crate::claude::path::{augmented_path, find_in_dirs, PATH_SEP}; #[derive(Debug, thiserror::Error)] pub enum CodexPathError { @@ -48,9 +48,11 @@ pub fn resolve_codex_path(override_bin: Option<&Path>) -> Result Vec { /// flattens `.` and `_` with them (`~/.brains/workspace` becomes /// `-Users-me--brains-desktop-workspace`). Guessing wrong here would cost a person their /// history, so both are asked and either counts. -fn encoded_cwds(cwd: &Path) -> Vec { - let raw = cwd.to_string_lossy().replace(['/', '\\'], "-"); +/// THE DRIVE COLON FLATTENS TOO, and leaving it out was a Windows bug with +/// teeth. `C:\Users\me\ws` encoded to `C:-Users-me-ws`, which still carries a +/// drive prefix — so `projects.join(encoded)` did not join at all. Windows +/// `Path::join` REPLACES the base when the argument is drive-qualified, and the +/// result resolved against the current directory on C:. The guard then counted +/// transcripts somewhere that was never the CLI's project dir, found none, and +/// reported no prior use — which is the one answer this module exists to get +/// right, because "no prior use" turns a wiped data dir into a fresh start +/// instead of a recovery offer. +/// +/// `C-` is also what the CLI itself writes: `C:\Repos\brains` becomes +/// `C--Repos-brains`, the colon and the separator each contributing one dash. +pub(super) fn encoded_cwds(cwd: &Path) -> Vec { + let raw = cwd.to_string_lossy().replace(['/', '\\', ':'], "-"); let flattened = raw.replace(['.', '_'], "-"); if flattened == raw { vec![raw] diff --git a/src/engines/storage/src/data_guard/tests.rs b/src/engines/storage/src/data_guard/tests.rs index 7518a0cd..ec063bd3 100644 --- a/src/engines/storage/src/data_guard/tests.rs +++ b/src/engines/storage/src/data_guard/tests.rs @@ -30,13 +30,20 @@ fn run(root: &Path, id: &str) { /// The two spellings of a cwd the Claude CLI has shipped. The guard must find /// transcripts under either, so the tests plant them under either. +/// THE PRODUCTION ENCODER, not a second copy of it. The copy that used to live +/// here spelled a Windows drive colon straight through, exactly as the code it +/// was checking did — so the two agreed, the test passed, and the fixture it +/// wrote landed in whatever directory C: was pointing at instead of under the +/// temp dir. A duplicated encoder can only ever confirm itself. fn encoded_cwd(cwd: &Path, flatten_dots: bool) -> String { - let raw = cwd.to_string_lossy().replace(['/', '\\'], "-"); + let spellings = super::evidence::encoded_cwds(cwd); if flatten_dots { - raw.replace(['.', '_'], "-") + spellings.first() } else { - raw + spellings.last() } + .expect("at least one spelling") + .clone() } fn cli_transcript(paths: &GuardPaths, flatten_dots: bool, name: &str) { @@ -178,6 +185,31 @@ fn transcripts_from_another_cwd_are_not_evidence() { fs::write(elsewhere.join("beef.jsonl"), "{}").unwrap(); assert_eq!(check_with(&paths, "1.0.0").unwrap(), GuardOutcome::Normal); + // The fixture went where it was told. A drive colon left in the encoding + // made this `join` throw its base away and resolve against the current + // directory instead, which is how this suite came to litter the source + // tree — and how the guard came to count transcripts in the wrong place. + assert!(elsewhere.starts_with(tmp.path()), "{}", elsewhere.display()); +} + +/// The CLI's spelling of a cwd, pinned on every platform because getting it +/// wrong is silent: the guard just counts zero transcripts and calls a wiped +/// install a first launch. A Windows path must come back with NO colon left in +/// it — one that survives keeps the name drive-qualified, and a drive-qualified +/// name is not a directory under `~/.claude/projects` at all. +#[test] +fn a_windows_drive_letter_is_flattened_out_of_the_encoding() { + let spellings = super::evidence::encoded_cwds(Path::new(r"C:\Repos\brains\workspace")); + assert!( + spellings.iter().all(|one| !one.contains(':')), + "a colon survived: {spellings:?}" + ); + // The colon and the separator each contribute one dash, which is what the + // CLI itself writes for this path. + assert!( + spellings.contains(&"C--Repos-brains-workspace".to_string()), + "{spellings:?}" + ); } /// An empty snapshot is not a life that was lost — a version bump on an install diff --git a/src/engines/storage/src/settings.rs b/src/engines/storage/src/settings.rs index 9056c8e4..efb6672f 100644 --- a/src/engines/storage/src/settings.rs +++ b/src/engines/storage/src/settings.rs @@ -282,7 +282,12 @@ impl SettingsFile { // settings.json written before the rename still carries a // workspaceRoot under it — and silently resurrects the dead path. // A relic override is treated as unset; anything else wins. + // `HOME` OR `USERPROFILE`, as `keychain.rs` reads both: `HOME` is a + // Unix name Windows sets only under Git Bash, so a GUI launch has + // just `USERPROFILE` and a `HOME`-only lookup never fires the guard. let is_legacy_relic = std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .filter(|home| !home.is_empty()) .map(|home| workspace_root.starts_with(Path::new(&home).join(".exo"))) .unwrap_or(false); if is_legacy_relic { @@ -584,8 +589,14 @@ mod tests { let tmp = tempfile::tempdir().unwrap(); let root = tmp.path().join(".brains"); std::fs::create_dir_all(&root).unwrap(); - let home = std::env::var("HOME").expect("test needs HOME"); - let relic = format!(r#"{{"workspaceRoot":"{home}/.exo/workspace"}}"#); + let home = std::env::var("HOME") + .or_else(|_| std::env::var("USERPROFILE")) + .expect("test needs HOME or USERPROFILE"); + // JSON, so a Windows home's backslashes have to be escaped. + let relic = format!( + r#"{{"workspaceRoot":"{}/.exo/workspace"}}"#, + home.replace('\\', "\\\\") + ); std::fs::write(root.join("settings.json"), relic).unwrap(); let loaded = load(&root.join("settings.json"), &root).unwrap(); diff --git a/src/layout/panes/session/__tests__/ledger-net.test.ts b/src/layout/panes/session/__tests__/ledger-net.test.ts index 7d043ead..0b3cf3b4 100644 --- a/src/layout/panes/session/__tests__/ledger-net.test.ts +++ b/src/layout/panes/session/__tests__/ledger-net.test.ts @@ -25,7 +25,7 @@ // audited is what the app sends. import { readFileSync, readdirSync } from "node:fs"; -import { join } from "node:path"; +import { join, relative, sep } from "node:path"; import { describe, expect, it } from "vitest"; import { withMentionContext, type Mention } from "$components/mentions"; import { ATTACHMENT_CONTEXT_PREFIX } from "$core/runtime/spine/attachments"; @@ -294,11 +294,22 @@ function code(text: string): string { .replace(/(^|[^:])\/\/.*$/gm, "$1"); } +/** A walked path as the BUILDERS list spells one: repo-relative, forward slashes. + * + * `join` follows the platform, so the same seven files come back + * `src\layout\…` on Windows and fail against a list written the way every other + * path in this tree is written. Same files, same content, one character — and a + * phantom failure that only a Windows dev ever sees is worse than no net at all, + * because it teaches them to distrust the one that works. */ +function repoPath(absolute: string): string { + return relative(process.cwd(), absolute).split(sep).join("/"); +} + describe("a builder the ledger has never heard of", () => { it("cannot reach the wire without being named here", () => { const builders = sources(SOURCE_ROOT) .filter((path) => BUILDS_A_BLOCK.test(code(readFileSync(path, "utf8")))) - .map((path) => path.slice(process.cwd().length + 1)) + .map(repoPath) .sort(); expect(builders).toEqual([...BUILDERS].sort());