From 556702efb89bc615e481d047e622c3440f7f7bb3 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Thu, 3 Sep 2026 14:17:34 -0700 Subject: [PATCH] windows: skills tests pass on Windows The two test temp roots kept `canonicalize`'s `\\?\` verbatim prefix, and the kernel takes a verbatim path literally, so every `/`-joined path built under them was an invalid filename. Strip it the way Node's `realpathSync` does. The manifest, artifact and sibling-binary expectations hard-coded POSIX separators for paths the product joins with the host's semantics; derive them from `jsp::join` instead. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Vau2X53xGTjjTCXWMVBoNY --- crates/skills/src/engine_binary.rs | 12 +++++- crates/skills/tests/drift_ports_tests.rs | 31 +++++++++++---- crates/skills/tests/hook_manifest_tests.rs | 38 ++++++++++++------- .../skills/tests/provider_commands_tests.rs | 6 ++- 4 files changed, 63 insertions(+), 24 deletions(-) diff --git a/crates/skills/src/engine_binary.rs b/crates/skills/src/engine_binary.rs index ea8de8c09..3e7f8a24a 100644 --- a/crates/skills/src/engine_binary.rs +++ b/crates/skills/src/engine_binary.rs @@ -174,7 +174,15 @@ mod tests { "https://github.com/pbakaus/impeccable/releases/download/engine-v1.2.3/impeccable-darwin-arm64" ); assert_eq!(asset_url("http://x/", "1", "windows", "x64"), "http://x/engine-v1/impeccable-windows-x64.exe"); - assert_eq!(binary_path("/s", "linux", "x64"), "/s/scripts/bin/linux-x64/impeccable"); - assert_eq!(binary_path("/s", "windows", "arm64"), "/s/scripts/bin/windows-arm64/impeccable.exe"); + // The sibling binary path is joined with the host's path semantics + // (backslashes on Windows); only the asset name is platform-keyed. + assert_eq!( + binary_path("/s", "linux", "x64"), + jsp::join(&["/s", "scripts", "bin", "linux-x64", "impeccable"]) + ); + assert_eq!( + binary_path("/s", "windows", "arm64"), + jsp::join(&["/s", "scripts", "bin", "windows-arm64", "impeccable.exe"]) + ); } } diff --git a/crates/skills/tests/drift_ports_tests.rs b/crates/skills/tests/drift_ports_tests.rs index e77392f1c..a53637cc8 100644 --- a/crates/skills/tests/drift_ports_tests.rs +++ b/crates/skills/tests/drift_ports_tests.rs @@ -16,7 +16,7 @@ use std::collections::HashMap; use std::path::PathBuf; -use impeccable_common::Io; +use impeccable_common::{jsp, Io}; use impeccable_skills::bundle::{self, download_file_with, FetchResponse}; use impeccable_skills::hook_manifest::merge_hook_manifests; use impeccable_skills::providers::{Scope, Sys}; @@ -29,7 +29,11 @@ fn temp_root(name: &str) -> String { .unwrap_or(0); let dir = std::env::temp_dir().join(format!("impeccable-{name}-{}-{nanos}", std::process::id())); std::fs::create_dir_all(&dir).expect("temp root"); - dir.canonicalize().unwrap().to_string_lossy().into_owned() + // Like Node's `realpathSync`: no `\\?\` verbatim prefix on Windows, so the + // `/`-joined paths these tests build under this root still resolve (the + // kernel takes a verbatim path literally and rejects a forward slash). + let real = dir.canonicalize().unwrap().to_string_lossy().into_owned(); + real.strip_prefix(r"\\?\").map(str::to_string).unwrap_or(real) } fn write(path: &str, content: &str) { @@ -284,7 +288,9 @@ fn download_file_http_initial_url_throws_without_calling_fetch() { #[test] fn local_bundle_uses_mkdtemp_staging_under_tmpdir() { let root = temp_root("staging"); - let tmp = format!("{root}/tmp"); + // The staging dir the bundle builds is joined with the host's path + // semantics, so the tmpdir it is compared against is joined the same way. + let tmp = jsp::join(&[&root, "tmp"]); std::fs::create_dir_all(&tmp).unwrap(); let bundle_root = create_fake_universal_bundle(&root, &[".claude"]); let env = base_env(&root, &tmp, &bundle_root); @@ -292,7 +298,7 @@ fn local_bundle_uses_mkdtemp_staging_under_tmpdir() { let staging = bundle::download_and_extract_bundle(&sys).unwrap(); assert!(staging.starts_with(&tmp), "{staging} not under {tmp}"); - let basename = staging.rsplit('/').next().unwrap(); + let basename = staging.rsplit(['/', '\\']).next().unwrap(); assert!(basename.starts_with("impeccable-local-bundle-"), "{basename}"); // Random mkdtemp suffix, not the old `--` form. let suffix = &basename["impeccable-local-bundle-".len()..]; @@ -453,6 +459,15 @@ fn inferred_home_rooted_updates_refresh_stale_or_missing_copilot_user_agents() { // ─── Grok project hooks on global installs (49571365, #642) ────────────────── +/// The launcher path the installer writes into a hook manifest, in the host's +/// path form and escaped the way it lands inside the JSON file (a Windows path +/// carries backslashes, which JSON doubles). +fn manifest_skill_path(home: &str, provider: &str) -> String { + let p = jsp::join(&[home, provider, "skills", "impeccable", "scripts", "impeccable"]); + let quoted = serde_json::to_string(&p).unwrap(); + quoted[1..quoted.len() - 1].to_string() +} + #[test] fn global_install_rewrites_grok_project_hooks_to_the_global_skill_path() { let root = temp_root("grok-global"); @@ -483,12 +498,12 @@ fn global_install_rewrites_grok_project_hooks_to_the_global_skill_path() { } // Launcher-era adaptation: the JS asserted the rewritten hook.mjs path; // the engine writes the launcher form pointing at the same global root. - assert!(read(&format!("{tmp}/.claude/settings.local.json")).contains(&format!("{home}/.claude/skills/impeccable/scripts/impeccable"))); - assert!(read(&format!("{tmp}/.codex/hooks.json")).contains(&format!("{home}/.agents/skills/impeccable/scripts/impeccable"))); - assert!(read(&format!("{tmp}/.cursor/hooks.json")).contains(&format!("{home}/.cursor/skills/impeccable/scripts/impeccable"))); + assert!(read(&format!("{tmp}/.claude/settings.local.json")).contains(&manifest_skill_path(&home, ".claude"))); + assert!(read(&format!("{tmp}/.codex/hooks.json")).contains(&manifest_skill_path(&home, ".agents"))); + assert!(read(&format!("{tmp}/.cursor/hooks.json")).contains(&manifest_skill_path(&home, ".cursor"))); let grok_hooks = read(&format!("{tmp}/.grok/hooks/impeccable.json")); assert!( - grok_hooks.contains(&format!("{home}/.grok/skills/impeccable/scripts/impeccable")), + grok_hooks.contains(&manifest_skill_path(&home, ".grok")), "grok hook not rewritten to the global skill path: {grok_hooks}" ); assert!( diff --git a/crates/skills/tests/hook_manifest_tests.rs b/crates/skills/tests/hook_manifest_tests.rs index 583100b94..6e0fafa60 100644 --- a/crates/skills/tests/hook_manifest_tests.rs +++ b/crates/skills/tests/hook_manifest_tests.rs @@ -2,6 +2,7 @@ //! hook command path resolution (#399)", "hook manifest merge helpers"), in //! the launcher generation. +use impeccable_common::jsp; use impeccable_skills::hook_manifest::*; use impeccable_skills::providers::Sys; use serde_json::{json, Value}; @@ -17,6 +18,12 @@ fn claude_bundle_manifest() -> Value { }) } +/// The launcher path a hook command points at, joined with the host's path +/// semantics the way `rewrite_hook_commands_for_platform` joins it. +fn skill_launcher(root: &str, provider: &str) -> String { + jsp::join(&[root, provider, "skills", "impeccable", "scripts", "impeccable"]) +} + fn commands(v: &Value) -> Vec { let mut out = Vec::new(); fn walk(v: &Value, out: &mut Vec) { @@ -63,7 +70,9 @@ fn absolute_path_is_single_quoted_and_inert_under_sh() { let root = "/tmp/imp-hook-$(touch pwned)-x"; let out = rewrite_hook_commands_for_platform(&claude_bundle_manifest(), ".claude", root, true, false); for c in commands(&out) { - let expected_path = format!("{root}/.claude/skills/impeccable/scripts/impeccable"); + // The launcher path is joined with the host's path semantics, so the + // expectation is joined the same way (backslashes on Windows). + let expected_path = skill_launcher(root, ".claude"); assert_eq!(c, format!("command=[ ! -f '{expected_path}' ] || '{expected_path}' hook")); assert!(!c.contains("${CLAUDE_PROJECT_DIR}")); assert!(!c.contains(&format!("\"{root}"))); @@ -89,7 +98,7 @@ fn windows_form_keeps_double_quoted_absolute_path() { let root = "/home/u"; let out = rewrite_hook_commands_for_platform(&claude_bundle_manifest(), ".claude", root, true, true); for c in commands(&out) { - let p = format!("{root}/.claude/skills/impeccable/scripts/impeccable"); + let p = skill_launcher(root, ".claude"); assert_eq!(c, format!("command=[ ! -f \"{p}\" ] || \"{p}\" hook")); assert!(!c.contains(&format!("'{p}"))); } @@ -147,9 +156,10 @@ fn github_manifests_pass_through_and_grok_is_rewritten() { "[ ! -f \".grok/skills/impeccable/scripts/impeccable\" ] || \".grok/skills/impeccable/scripts/impeccable\" hook" ); let abs = rewrite_hook_commands_for_platform(&grok, ".grok", "/home/u", true, false); + let p = skill_launcher("/home/u", ".grok"); assert_eq!( abs["hooks"]["PostToolUse"][0]["hooks"][0]["command"], - "[ ! -f '/home/u/.grok/skills/impeccable/scripts/impeccable' ] || '/home/u/.grok/skills/impeccable/scripts/impeccable' hook" + serde_json::Value::String(format!("[ ! -f '{p}' ] || '{p}' hook")) ); // Grok is not Codex: no commandWindows sibling is added. assert!(rel["hooks"]["PostToolUse"][0]["hooks"][0].get("commandWindows").is_none()); @@ -326,19 +336,21 @@ fn repair_leaves_dirs_without_an_impeccable_marker_alone() { #[test] fn hook_artifacts_map_providers_to_manifest_files() { let dests = expected_hook_dests("/p", &[".claude", ".agents", ".cursor", ".github", ".grok", ".gemini"]); + // Manifest destinations are joined with the host's path semantics, so the + // expectations are joined the same way (backslashes on Windows). assert_eq!(dests, [ - "/p/.claude/settings.local.json", - "/p/.codex/hooks.json", - "/p/.cursor/hooks.json", - "/p/.github/hooks/impeccable.json", - "/p/.grok/hooks/impeccable.json", + jsp::join(&["/p", ".claude", "settings.local.json"]), + jsp::join(&["/p", ".codex", "hooks.json"]), + jsp::join(&["/p", ".cursor", "hooks.json"]), + jsp::join(&["/p", ".github", "hooks", "impeccable.json"]), + jsp::join(&["/p", ".grok", "hooks", "impeccable.json"]), ]); let a = hook_artifacts_for_provider("/b", "/p", ".claude"); - assert_eq!(a[0].src, "/b/.claude/settings.json"); - assert_eq!(a[0].dest, "/p/.claude/settings.local.json"); - assert_eq!(a[0].shared_dest.as_deref(), Some("/p/.claude/settings.json")); + assert_eq!(a[0].src, jsp::join(&["/b", ".claude", "settings.json"])); + assert_eq!(a[0].dest, jsp::join(&["/p", ".claude", "settings.local.json"])); + assert_eq!(a[0].shared_dest.as_deref(), Some(jsp::join(&["/p", ".claude", "settings.json"]).as_str())); let c = hook_artifacts_for_provider("/b", "/p", ".agents"); - assert_eq!(c[0].src, "/b/.codex/hooks.json"); - assert_eq!(c[0].dest, "/p/.codex/hooks.json"); + assert_eq!(c[0].src, jsp::join(&["/b", ".codex", "hooks.json"])); + assert_eq!(c[0].dest, jsp::join(&["/p", ".codex", "hooks.json"])); assert!(c[0].shared_dest.is_none()); } diff --git a/crates/skills/tests/provider_commands_tests.rs b/crates/skills/tests/provider_commands_tests.rs index b50354d5b..673d227c6 100644 --- a/crates/skills/tests/provider_commands_tests.rs +++ b/crates/skills/tests/provider_commands_tests.rs @@ -15,7 +15,11 @@ fn temp_root(name: &str) -> String { .unwrap_or(0); let dir = std::env::temp_dir().join(format!("impeccable-{name}-{}-{nanos}", std::process::id())); std::fs::create_dir_all(&dir).expect("temp root"); - dir.canonicalize().unwrap().to_string_lossy().into_owned() + // Like Node's `realpathSync`: no `\\?\` verbatim prefix on Windows, so the + // `/`-joined paths these tests build under this root still resolve (the + // kernel takes a verbatim path literally and rejects a forward slash). + let real = dir.canonicalize().unwrap().to_string_lossy().into_owned(); + real.strip_prefix(r"\\?\").map(str::to_string).unwrap_or(real) } fn write(path: &str, content: &str) {