W2: review-handoff fixes + simplify pass

Addresses both findings from the code-review of #10 and the subsequent
/simplify sweep, per agentic-flow steps 07/09/10.

Review fixes:
- atomic_write now preserves the destination's prior Unix mode rather
  than silently downgrading .claude/settings.json to NamedTempFile's
  0o600 default. Fresh files default to 0o644; the hook script
  continues to land at 0o755. Three new tests in install_claude_code.rs
  pin the mode contract.
- Tighten the doc-comment on unmerge_hook_entry to admit the round-trip
  caveat: a pre-existing {matcher: "Bash", hooks: []} placeholder is
  also dropped because we can't recover provenance. The corresponding
  inline comment now points at the doc.

/simplify pass:
- atomic_write takes mode: u32 instead of Option<u32>; the None branch
  was dead.
- get_or_insert_object / get_or_insert_array harmonised to (map, key,
  path) — eliminates the asymmetric "path derived from key" rule.
- JSON-schema keys ("hooks", "PreToolUse", "matcher", "Bash", "type",
  "command") are now module-level constants in settings.rs. Stops a
  typo from becoming a silent no-op.
- Drop a few comments that restated obvious behaviour (HookState
  variant docs, narrating-the-change comments in tests, POSIX rationale
  on a single-line newline assertion, task-reference in the
  fallow-fixture test header).

cargo fmt + clippy --workspace -- -D warnings + cargo test --workspace
all green: 70 passing, 1 ignored (W1's documented git-dash-c case).

Author: liam-ai-reality

klasp-agents-claude/src/hook_template.rs

@@ -47,9 +47,6 @@ mod tests {
 
     #[test]
     fn render_ends_with_newline() {
-        // POSIX: text files end with newlines. The script gets `chmod +x`'d
-        // and an interactive editor that does "ensure final newline" should
-        // produce no diff.
         assert!(render(1).ends_with('\n'));
     }
 }

klasp-agents-claude/src/settings.rs

@@ -10,6 +10,16 @@
 use serde_json::{Map, Value};
 use thiserror::Error;
 
+// Claude Code's `.claude/settings.json` hook schema. These keys appear
+// across the merge / unmerge / lookup helpers; pinning them as constants
+// turns a typo into a compile error instead of a silent no-op.
+const HOOKS: &str = "hooks";
+const PRETOOL_USE: &str = "PreToolUse";
+const MATCHER: &str = "matcher";
+const TYPE: &str = "type";
+const COMMAND: &str = "command";
+const BASH: &str = "Bash";
+
 #[derive(Debug, Error)]
 pub enum SettingsError {
     #[error("settings.json: invalid JSON: {0}")]
@@ -43,42 +53,52 @@ pub fn merge_hook_entry(settings_json: &str, hook_command: &str) -> Result<Strin
 
     let root_obj = expect_object_mut(&mut root, "")?;
 
-    let hooks = get_or_insert_object(root_obj, "hooks")?;
-    let pretool = get_or_insert_array(hooks, "PreToolUse", "hooks.PreToolUse")?;
+    let hooks = get_or_insert_object(root_obj, HOOKS, HOOKS)?;
+    let pretool = get_or_insert_array(hooks, PRETOOL_USE, "hooks.PreToolUse")?;
 
     let bash_idx = find_bash_matcher(pretool)?;
     let bash_entry = match bash_idx {
         Some(i) => &mut pretool[i],
         None => {
             pretool.push(Value::Object({
                 let mut m = Map::new();
-                m.insert("matcher".into(), Value::String("Bash".into()));
-                m.insert("hooks".into(), Value::Array(Vec::new()));
+                m.insert(MATCHER.into(), Value::String(BASH.into()));
+                m.insert(HOOKS.into(), Value::Array(Vec::new()));
                 m
             }));
             pretool.last_mut().expect("just pushed")
         }
     };
 
     let bash_obj = expect_object_mut(bash_entry, "hooks.PreToolUse[Bash]")?;
-    let inner = get_or_insert_array(bash_obj, "hooks", "hooks.PreToolUse[Bash].hooks")?;
+    let inner = get_or_insert_array(bash_obj, HOOKS, "hooks.PreToolUse[Bash].hooks")?;
 
     if !inner.iter().any(|h| hook_command_matches(h, hook_command)) {
         let mut entry = Map::new();
-        entry.insert("type".into(), Value::String("command".into()));
-        entry.insert("command".into(), Value::String(hook_command.into()));
+        entry.insert(TYPE.into(), Value::String(COMMAND.into()));
+        entry.insert(COMMAND.into(), Value::String(hook_command.into()));
         inner.push(Value::Object(entry));
     }
 
     Ok(serialise(&root))
 }
 
 /// Inverse of [`merge_hook_entry`]: remove every hook with `command` exactly
-/// equal to `hook_command`. Cleans up empty arrays/objects so a fresh-install
-/// settings.json round-trips through install→uninstall to its pre-install shape.
+/// equal to `hook_command`, then clean up the empty `Bash` matcher and
+/// surrounding `hooks.PreToolUse` / `hooks` containers when they're left
+/// empty. After install→uninstall, an `.claude/settings.json` that had no
+/// pre-existing hook content returns to `{}`.
+///
+/// Round-trip is best-effort, not byte-identical: serialised output uses
+/// 2-space pretty-print + trailing newline regardless of pre-install
+/// formatting. A pre-existing `{matcher: "Bash", hooks: []}` placeholder
+/// is also dropped — we can't distinguish "klasp emptied this" from
+/// "user pre-installed an empty placeholder" — but that shape is rare in
+/// practice. Non-Bash matchers and Bash matchers with surviving sibling
+/// hooks are preserved.
 ///
-/// Idempotent: running on a settings.json that has no klasp entry returns the
-/// input unchanged.
+/// Idempotent: running on a settings.json that has no klasp entry parses
+/// and re-serialises (whitespace may differ but JSON content is unchanged).
 pub fn unmerge_hook_entry(
     settings_json: &str,
     hook_command: &str,
@@ -91,12 +111,12 @@ pub fn unmerge_hook_entry(
     let mut root: Value = serde_json::from_str(trimmed)?;
     let root_obj = expect_object_mut(&mut root, "")?;
 
-    let Some(hooks_val) = root_obj.get_mut("hooks") else {
+    let Some(hooks_val) = root_obj.get_mut(HOOKS) else {
         return Ok(serialise(&root));
     };
-    let hooks = expect_object_mut(hooks_val, "hooks")?;
+    let hooks = expect_object_mut(hooks_val, HOOKS)?;
 
-    let Some(pretool_val) = hooks.get_mut("PreToolUse") else {
+    let Some(pretool_val) = hooks.get_mut(PRETOOL_USE) else {
         return Ok(serialise(&root));
     };
     let pretool = expect_array_mut(pretool_val, "hooks.PreToolUse")?;
@@ -105,7 +125,7 @@ pub fn unmerge_hook_entry(
         let Some(matcher_obj) = matcher.as_object_mut() else {
             continue;
         };
-        let Some(inner_val) = matcher_obj.get_mut("hooks") else {
+        let Some(inner_val) = matcher_obj.get_mut(HOOKS) else {
             continue;
         };
         let Some(inner) = inner_val.as_array_mut() else {
@@ -114,28 +134,26 @@ pub fn unmerge_hook_entry(
         inner.retain(|h| !hook_command_matches(h, hook_command));
     }
 
-    // Sweep up Bash matchers whose `hooks` array is now empty (klasp put
-    // them there in the first place). Untouched matchers — those that
-    // started with sibling hooks — keep their `hooks: []` if a teammate
-    // emptied them, since that's the user's data.
+    // See the function doc-comment for why pre-existing empty Bash
+    // placeholders are also swept (provenance is unrecoverable).
     pretool.retain(|m| {
         let Some(obj) = m.as_object() else {
             return true;
         };
-        if obj.get("matcher").and_then(Value::as_str) != Some("Bash") {
+        if obj.get(MATCHER).and_then(Value::as_str) != Some(BASH) {
             return true;
         }
         !matches!(
-            obj.get("hooks").and_then(Value::as_array),
+            obj.get(HOOKS).and_then(Value::as_array),
             Some(arr) if arr.is_empty()
         )
     });
 
     if pretool.is_empty() {
-        hooks.remove("PreToolUse");
+        hooks.remove(PRETOOL_USE);
     }
     if hooks.is_empty() {
-        root_obj.remove("hooks");
+        root_obj.remove(HOOKS);
     }
 
     Ok(serialise(&root))
@@ -174,11 +192,12 @@ fn expect_array_mut<'a>(
 fn get_or_insert_object<'a>(
     map: &'a mut Map<String, Value>,
     key: &str,
+    path: &str,
 ) -> Result<&'a mut Map<String, Value>, SettingsError> {
     let entry = map.entry(key).or_insert_with(|| Value::Object(Map::new()));
     let got = describe(entry);
     entry.as_object_mut().ok_or(SettingsError::Shape {
-        path: key.to_string(),
+        path: path.to_string(),
         expected: "object",
         got,
     })
@@ -187,20 +206,17 @@ fn get_or_insert_object<'a>(
 fn get_or_insert_array<'a>(
     map: &'a mut Map<String, Value>,
     key: &str,
-    full_path: &str,
+    path: &str,
 ) -> Result<&'a mut Vec<Value>, SettingsError> {
     let entry = map.entry(key).or_insert_with(|| Value::Array(Vec::new()));
     let got = describe(entry);
     entry.as_array_mut().ok_or(SettingsError::Shape {
-        path: full_path.to_string(),
+        path: path.to_string(),
         expected: "array",
         got,
     })
 }
 
-/// Find the index of a matcher object in `PreToolUse` whose `matcher` field
-/// is exactly the string `"Bash"`. Errors only if a matcher entry isn't an
-/// object (Claude's schema requires it).
 fn find_bash_matcher(pretool: &[Value]) -> Result<Option<usize>, SettingsError> {
     for (i, m) in pretool.iter().enumerate() {
         let Some(obj) = m.as_object() else {
@@ -210,15 +226,15 @@ fn find_bash_matcher(pretool: &[Value]) -> Result<Option<usize>, SettingsError>
                 got: describe(m),
             });
         };
-        if obj.get("matcher").and_then(Value::as_str) == Some("Bash") {
+        if obj.get(MATCHER).and_then(Value::as_str) == Some(BASH) {
             return Ok(Some(i));
         }
     }
     Ok(None)
 }
 
 fn hook_command_matches(hook: &Value, expected_command: &str) -> bool {
-    hook.get("command").and_then(Value::as_str) == Some(expected_command)
+    hook.get(COMMAND).and_then(Value::as_str) == Some(expected_command)
 }
 
 fn describe(v: &Value) -> &'static str {
@@ -436,8 +452,6 @@ mod tests {
         );
         let out = unmerge_hook_entry(&input, KLASP_CMD).unwrap();
         let v = parse(&out);
-        // hooks key should be gone (the only matcher was klasp's, which we
-        // then dropped because its hooks array became empty).
         assert!(v.get("hooks").is_none(), "got: {v:#?}");
     }
 

klasp-agents-claude/src/surface.rs

@@ -86,14 +86,16 @@ impl AgentSurface for ClaudeCodeSurface {
 
         if !matches!(hook_state, HookState::Identical) {
             ensure_parent(&hook_path)?;
-            atomic_write(&hook_path, rendered.as_bytes())?;
-            set_executable(&hook_path)?;
+            atomic_write(&hook_path, rendered.as_bytes(), 0o755)?;
             paths_written.push(hook_path.clone());
         }
 
         if !settings_unchanged {
             ensure_parent(&settings_path)?;
-            atomic_write(&settings_path, merged.as_bytes())?;
+            // Preserve the user's prior mode rather than overwriting it with
+            // NamedTempFile's 0o600 default; fall back to 0o644 for new files.
+            let mode = current_mode(&settings_path).unwrap_or(0o644);
+            atomic_write(&settings_path, merged.as_bytes(), mode)?;
             paths_written.push(settings_path.clone());
         }
 
@@ -137,7 +139,8 @@ impl AgentSurface for ClaudeCodeSurface {
                 .map_err(|e| settings_error(&settings_path, e))?;
             if new != existing {
                 if !dry_run {
-                    atomic_write(&settings_path, new.as_bytes())?;
+                    let mode = current_mode(&settings_path).unwrap_or(0o644);
+                    atomic_write(&settings_path, new.as_bytes(), mode)?;
                 }
                 paths.push(settings_path);
             }
@@ -148,10 +151,7 @@ impl AgentSurface for ClaudeCodeSurface {
 }
 
 enum HookState {
-    /// File already exists and matches the rendered output byte-for-byte.
     Identical,
-    /// File doesn't exist, or exists with the marker but different content
-    /// (e.g. older schema version) — safe to overwrite.
     Writable,
 }
 
@@ -202,7 +202,10 @@ fn ensure_parent(path: &Path) -> Result<(), InstallError> {
     })
 }
 
-fn atomic_write(path: &Path, contents: &[u8]) -> Result<(), InstallError> {
+/// Atomic write via tempfile + rename. `mode` is applied after the rename
+/// (Unix only) — without it the destination silently inherits
+/// `NamedTempFile`'s `0o600` default.
+fn atomic_write(path: &Path, contents: &[u8], mode: u32) -> Result<(), InstallError> {
     let dir = path.parent().unwrap_or_else(|| Path::new("."));
     let mut tf = tempfile::NamedTempFile::new_in(dir).map_err(|e| InstallError::Io {
         path: dir.to_path_buf(),
@@ -220,20 +223,29 @@ fn atomic_write(path: &Path, contents: &[u8]) -> Result<(), InstallError> {
         path: path.to_path_buf(),
         source: e.error,
     })?;
+    apply_mode(path, mode)?;
     Ok(())
 }
 
-fn set_executable(path: &Path) -> Result<(), InstallError> {
+/// The file's current Unix mode (low 12 bits), or `None` if the file
+/// doesn't exist or we're not on Unix. Called *before* `atomic_write`
+/// so we can restore the user's prior mode after the rename.
+#[cfg(unix)]
+fn current_mode(path: &Path) -> Option<u32> {
+    use std::os::unix::fs::PermissionsExt;
+    fs::metadata(path).ok().map(|m| m.permissions().mode())
+}
+
+#[cfg(not(unix))]
+fn current_mode(_path: &Path) -> Option<u32> {
+    None
+}
+
+fn apply_mode(path: &Path, mode: u32) -> Result<(), InstallError> {
     #[cfg(unix)]
     {
         use std::os::unix::fs::PermissionsExt;
-        let mut perms = fs::metadata(path)
-            .map_err(|e| InstallError::Io {
-                path: path.to_path_buf(),
-                source: e,
-            })?
-            .permissions();
-        perms.set_mode(0o755);
+        let perms = std::fs::Permissions::from_mode(mode);
         fs::set_permissions(path, perms).map_err(|e| InstallError::Io {
             path: path.to_path_buf(),
             source: e,
@@ -243,7 +255,7 @@ fn set_executable(path: &Path) -> Result<(), InstallError> {
     {
         // Windows: bash interprets the shebang regardless of the NTFS bit.
         // Per design.md §14, the Windows path/permission audit lands at W4.
-        let _ = path;
+        let _ = (path, mode);
     }
     Ok(())
 }

klasp-agents-claude/tests/settings_merge.rs

@@ -1,7 +1,6 @@
-//! Integration test: merge into a realistic fallow-shaped `.claude/settings.json`.
-//!
-//! Per the W2 issue (#2): "Unit tests for settings.json merge with a real
-//! fallow settings.json fixture (proves sibling hooks survive)".
+//! Integration test: merge into a realistic fallow-shaped
+//! `.claude/settings.json`, proving every sibling hook entry and
+//! top-level key survives byte-for-byte.
 
 use klasp_agents_claude::ClaudeCodeSurface;
 use klasp_agents_claude::{merge_hook_entry, unmerge_hook_entry};

klasp/tests/install_claude_code.rs

@@ -187,6 +187,65 @@ fn uninstall_removes_klasp_and_preserves_siblings() {
     assert_eq!(inner[0]["command"], "fallow gate");
 }
 
+#[cfg(unix)]
+#[test]
+fn install_preserves_existing_settings_mode() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let repo = fresh_repo();
+    let settings_path = repo.path().join(".claude/settings.json");
+    fs::write(&settings_path, "{}").unwrap();
+    fs::set_permissions(&settings_path, fs::Permissions::from_mode(0o644)).unwrap();
+
+    ClaudeCodeSurface
+        .install(&ctx_for(repo.path(), false))
+        .unwrap();
+
+    let mode = fs::metadata(&settings_path).unwrap().permissions().mode() & 0o777;
+    assert_eq!(
+        mode, 0o644,
+        "atomic_write must preserve existing settings.json mode, not downgrade to NamedTempFile's 0o600 default"
+    );
+}
+
+#[cfg(unix)]
+#[test]
+fn install_creates_fresh_settings_with_0o644() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let repo = fresh_repo();
+    let settings_path = repo.path().join(".claude/settings.json");
+    assert!(!settings_path.exists(), "fresh repo has no settings.json");
+
+    ClaudeCodeSurface
+        .install(&ctx_for(repo.path(), false))
+        .unwrap();
+
+    let mode = fs::metadata(&settings_path).unwrap().permissions().mode() & 0o777;
+    assert_eq!(
+        mode, 0o644,
+        "freshly-created settings.json should land at 0o644, not the 0o600 default of NamedTempFile"
+    );
+}
+
+#[cfg(unix)]
+#[test]
+fn install_writes_hook_with_0o755() {
+    use std::os::unix::fs::PermissionsExt;
+
+    let repo = fresh_repo();
+    ClaudeCodeSurface
+        .install(&ctx_for(repo.path(), false))
+        .unwrap();
+
+    let mode = fs::metadata(repo.path().join(".claude/hooks/klasp-gate.sh"))
+        .unwrap()
+        .permissions()
+        .mode()
+        & 0o777;
+    assert_eq!(mode, 0o755);
+}
+
 #[test]
 fn uninstall_dry_run_writes_nothing() {
     let repo = fresh_repo();