From 50d130dd97d55c828433e317cf5f657be48db69a Mon Sep 17 00:00:00 2001 From: Abdul Wahab Date: Tue, 8 Sep 2026 19:23:50 +0500 Subject: [PATCH] Fix: user-scope Windows hook group duplication (#784) JSON-quoted absolute paths doubled backslashes, so merge failed to recognize the group it had just written and appended another copy on every update. Prepared with AI assistance. Co-authored-by: Cursor --- crates/context/src/hook_markers.rs | 67 ++++++++++++++++++---- crates/skills/src/hook_manifest.rs | 12 +--- crates/skills/tests/hook_manifest_tests.rs | 54 +++++++++++++++++ 3 files changed, 113 insertions(+), 20 deletions(-) diff --git a/crates/context/src/hook_markers.rs b/crates/context/src/hook_markers.rs index 9cddc70c9..ffa4baa5e 100644 --- a/crates/context/src/hook_markers.rs +++ b/crates/context/src/hook_markers.rs @@ -30,9 +30,30 @@ static LAUNCHER_HOOK_MARKER: Lazy = Lazy::new(|| { Regex::new(r#"skills/impeccable/scripts/impeccable(?:\.cmd|\.exe)?["']?\s+hook(?:-before-edit|-probe|-after-edit|-stop)?(?:\s|$|["'&|;)])"#).unwrap() }); +/// User-scope Windows commands embed a JSON-quoted path whose backslashes are +/// doubled in the command string (#784). #604's single `\`→`/` replace is not +/// enough: `\\` becomes `//`, which breaks `skills/impeccable` matching. +fn normalize_hook_separators(command: &str) -> String { + let mut out = String::with_capacity(command.len()); + let mut last_was_sep = false; + for ch in command.chars() { + if ch == '\\' || ch == '/' { + if !last_was_sep { + out.push('/'); + last_was_sep = true; + } + } else { + out.push(ch); + last_was_sep = false; + } + } + out +} + /// True when `command` invokes an Impeccable hook in either generation's spelling. pub fn is_impeccable_hook_command(command: &str) -> bool { - LEGACY_HOOK_SCRIPT_MARKERS.iter().any(|m| command.contains(m)) || LAUNCHER_HOOK_MARKER.is_match(command) + let command = normalize_hook_separators(command); + LEGACY_HOOK_SCRIPT_MARKERS.iter().any(|m| command.contains(m)) || LAUNCHER_HOOK_MARKER.is_match(&command) } /// True when `command` invokes an Impeccable hook in the launcher generation @@ -40,7 +61,7 @@ pub fn is_impeccable_hook_command(command: &str) -> bool { /// one: install/update use this to decide which manifests still need /// migrating to the launcher form. pub fn is_launcher_hook_command(command: &str) -> bool { - LAUNCHER_HOOK_MARKER.is_match(command) + LAUNCHER_HOOK_MARKER.is_match(&normalize_hook_separators(command)) } /// The launcher-era markers `context` and `doctor` treat as the design hook @@ -54,9 +75,10 @@ static LAUNCHER_DESIGN_HOOK: Lazy = Lazy::new(|| { /// `hook-before-edit`) in either spelling; the JS `context.mjs` scan and /// `staleness-deep` HOOK_SCRIPT_MARKERS both meant exactly these two. pub fn is_design_hook_command(command: &str) -> bool { + let command = normalize_hook_separators(command); command.contains("skills/impeccable/scripts/hook.mjs") || command.contains("skills/impeccable/scripts/hook-before-edit.mjs") - || LAUNCHER_DESIGN_HOOK.is_match(command) + || LAUNCHER_DESIGN_HOOK.is_match(&command) } /// True when `command` runs the design hook (`hook` / `hook-before-edit`) in @@ -66,7 +88,7 @@ pub fn is_design_hook_command(command: &str) -> bool { /// so the hook is dead and `MANUAL_DETECTOR_REQUIRED` must fire until an /// install/update repairs it. pub fn is_launcher_design_hook_command(command: &str) -> bool { - LAUNCHER_DESIGN_HOOK.is_match(command) + LAUNCHER_DESIGN_HOOK.is_match(&normalize_hook_separators(command)) } /// The shell token that names the hook program inside `command`, for @@ -74,7 +96,11 @@ pub fn is_launcher_design_hook_command(command: &str) -> bool { /// path in the binary form. `None` when the command carries no marker or /// the token cannot be isolated (a `'\''` escape sequence, for instance). pub fn hook_program_token(command: &str) -> Option { - if !is_design_hook_command(command) { + if command.contains("'\\''") { + return None; + } + let command = normalize_hook_separators(command); + if !is_design_hook_command(&command) { return None; } static QUOTED: Lazy = Lazy::new(|| { @@ -86,16 +112,13 @@ pub fn hook_program_token(command: &str) -> Option { static BARE: Lazy = Lazy::new(|| { Regex::new(r#"([^\s"'|&;()]*skills/impeccable/scripts/(?:hook(?:-before-edit)?\.mjs|impeccable(?:\.cmd|\.exe)?))"#).unwrap() }); - if let Some(m) = QUOTED.captures(command) { + if let Some(m) = QUOTED.captures(&command) { return Some(m[1].to_string()); } - if command.contains("'\\''") { - return None; - } - if let Some(m) = SINGLE.captures(command) { + if let Some(m) = SINGLE.captures(&command) { return Some(m[1].to_string()); } - BARE.captures(command).map(|m| m[1].to_string()) + BARE.captures(&command).map(|m| m[1].to_string()) } #[cfg(test)] @@ -183,4 +206,26 @@ mod tests { assert_eq!(hook_program_token("'/x/it'\\''s/.claude/skills/impeccable/scripts/impeccable' hook"), None); assert_eq!(hook_program_token("echo hi"), None); } + + #[test] + fn recognizes_json_escaped_windows_launcher_path() { + let launcher = r"C:\Users\alice\.claude\skills\impeccable\scripts\impeccable"; + let quoted = serde_json::to_string(launcher).unwrap(); + let cmd = format!("[ ! -f {quoted} ] || {quoted} hook"); + assert!(is_impeccable_hook_command(&cmd), "{cmd}"); + assert!(is_launcher_hook_command(&cmd), "{cmd}"); + assert!(is_design_hook_command(&cmd), "{cmd}"); + assert_eq!( + hook_program_token(&cmd).as_deref(), + Some("C:/Users/alice/.claude/skills/impeccable/scripts/impeccable") + ); + } + + #[test] + fn recognizes_single_backslash_windows_path() { + let cmd = r#"[ ! -f "C:\Users\alice\.claude\skills\impeccable\scripts\impeccable" ] || "C:\Users\alice\.claude\skills\impeccable\scripts\impeccable" hook"#; + assert!(is_impeccable_hook_command(cmd), "{cmd}"); + assert!(is_launcher_hook_command(cmd), "{cmd}"); + assert!(is_design_hook_command(cmd), "{cmd}"); + } } diff --git a/crates/skills/src/hook_manifest.rs b/crates/skills/src/hook_manifest.rs index a546c94e3..a12687f68 100644 --- a/crates/skills/src/hook_manifest.rs +++ b/crates/skills/src/hook_manifest.rs @@ -219,13 +219,10 @@ fn rewrite_value(value: &Value, provider: &str, quoted: &QuotedPath, win32: bool } } -/// JS: valueHasImpeccableHookMarker(value). Command separators are -/// normalized to `/` first so a legacy Windows-path guard is still -/// recognized as ours and replaced instead of duplicated (upstream -/// 665c51b9, #604). +/// JS: valueHasImpeccableHookMarker(value). pub fn value_has_impeccable_hook_marker(value: &Value) -> bool { match value { - Value::String(s) => is_impeccable_hook_command(&s.replace('\\', "/")), + Value::String(s) => is_impeccable_hook_command(s), Value::Array(a) => a.iter().any(value_has_impeccable_hook_marker), Value::Object(o) => o.values().any(value_has_impeccable_hook_marker), _ => false, @@ -233,12 +230,9 @@ pub fn value_has_impeccable_hook_marker(value: &Value) -> bool { } /// True when `value` names an Impeccable hook in the launcher generation. -/// Separators are normalized to `/` first, matching -/// `value_has_impeccable_hook_marker`, so a legacy Windows-path launcher -/// command is still recognized. pub fn value_has_launcher_hook_marker(value: &Value) -> bool { match value { - Value::String(s) => is_launcher_hook_command(&s.replace('\\', "/")), + Value::String(s) => is_launcher_hook_command(s), Value::Array(a) => a.iter().any(value_has_launcher_hook_marker), Value::Object(o) => o.values().any(value_has_launcher_hook_marker), _ => false, diff --git a/crates/skills/tests/hook_manifest_tests.rs b/crates/skills/tests/hook_manifest_tests.rs index 81aef3c90..c693dcd86 100644 --- a/crates/skills/tests/hook_manifest_tests.rs +++ b/crates/skills/tests/hook_manifest_tests.rs @@ -357,3 +357,57 @@ fn hook_artifacts_map_providers_to_manifest_files() { assert_eq!(c[0].dest, jsp::join(&["/p", ".codex", "hooks.json"])); assert!(c[0].shared_dest.is_none()); } + +fn windows_user_scope_hook_command() -> String { + let launcher = r"C:\Users\alice\.claude\skills\impeccable\scripts\impeccable"; + let q = json_string(launcher); + format!("[ ! -f {q} ] || {q} hook") +} + +#[test] +fn merge_json_escaped_windows_launcher_is_idempotent() { + let cmd = windows_user_scope_hook_command(); + let hook_entry = |cmd: String| { + json!({ "matcher": "Edit", "hooks": [{ "type": "command", "command": cmd }] }) + }; + let stop_entry = |cmd: String| { + json!({ "hooks": [{ "type": "command", "command": cmd, "timeout": 30 }] }) + }; + let existing = json!({ + "hooks": { + "PostToolUse": [hook_entry(cmd.clone())], + "Stop": [stop_entry(cmd.clone())] + } + }); + let fresh = json!({ + "description": "fresh", + "hooks": { + "PostToolUse": [hook_entry(cmd.clone())], + "Stop": [stop_entry(cmd.clone())] + } + }); + let merged = merge_hook_manifests(&existing, &fresh); + assert_eq!(merged["hooks"]["PostToolUse"].as_array().unwrap().len(), 1); + assert_eq!(merged["hooks"]["Stop"].as_array().unwrap().len(), 1); + let merged2 = merge_hook_manifests(&merged, &fresh); + assert_eq!(merged2["hooks"]["PostToolUse"].as_array().unwrap().len(), 1); + assert_eq!(merged2["hooks"]["Stop"].as_array().unwrap().len(), 1); +} + +#[test] +fn merge_heals_triplicated_stop_groups() { + let cmd = windows_user_scope_hook_command(); + let stop_entry = json!({ "hooks": [{ "type": "command", "command": cmd.clone(), "timeout": 30 }] }); + let existing = json!({ + "hooks": { + "Stop": [stop_entry.clone(), stop_entry.clone(), stop_entry] + } + }); + let fresh = json!({ + "hooks": { + "Stop": [json!({ "hooks": [{ "type": "command", "command": cmd, "timeout": 30 }] })] + } + }); + let merged = merge_hook_manifests(&existing, &fresh); + assert_eq!(merged["hooks"]["Stop"].as_array().unwrap().len(), 1); +}