fix: preserve claude settings key order

[akbash-bot]

Summary

• update Claude settings through a strict, lossless CST instead of reserializing the whole file
• preserve key order, whitespace, compact layouts, line endings, scalar spelling, and trailing suffixes outside Herdr-owned hook edits
• keep install and uninstall byte-idempotent while verifying the edited file matches the existing semantic migration behavior

Checks

• just check

refs #2066

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Claude hook installation and removal now use dedicated CST-based helpers. The implementation validates settings, preserves formatting and unchanged bytes, updates managed hooks selectively, and routes target operations through the new helpers.

Changes

Claude settings migration

Layer / File(s)	Summary
CST rewrite and validation Cargo.toml, src/integration/mod.rs, src/integration/claude_settings.rs	Adds jsonc-parser support, strict JSON validation, duplicate-key detection, and formatting-preserving CST edits.
Hook transformation and integration src/integration/claude_settings.rs, src/integration/targets.rs	Adds separate install and uninstall operations. Claude target operations use these helpers and write settings only when content changes.
Hook transformation validation src/integration/claude_settings.rs	Tests formatting preservation, compact and multiline containers, idempotence, selective removal, uninstallation, duplicate keys, and invalid input.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant targets
  participant claude_settings
  participant settings_content
  targets->>claude_settings: call install or uninstall
  claude_settings->>settings_content: parse and edit settings through CST
  claude_settings-->>targets: return original or updated content
  targets->>settings_content: write only when content changed

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: preserving key order in Claude settings.
Description check	✅ Passed	The description accurately explains the lossless CST changes and preservation behavior for Claude settings.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch akbash/2066-preserve-claude-settings

---

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR replaces whole-file Claude settings serialization with targeted CST-based edits.

• Preserves key order, formatting, scalar spelling, line endings, and trailing content outside Herdr-owned hook changes.
• Keeps install and uninstall byte-idempotent when no semantic update is required.
• Adds jsonc-parser for strict CST parsing and routes Claude integration updates through the new editor.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
src/integration/claude_settings.rs	Introduces strict CST-based Claude hook migration with semantic verification and formatting-preservation tests.
src/integration/targets.rs	Routes Claude installation and removal through the lossless settings editor and avoids unchanged writes.
Cargo.toml	Adds jsonc-parser with only the CST and serde_json features required by the new editor.
Cargo.lock	Locks jsonc-parser 0.33.1 and its existing serde_json dependency.
src/integration/mod.rs	Registers the new Claude settings module.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Read Claude settings.json] --> B[Parse semantic JSON]
    B --> C[Compute desired hook migration]
    C --> D{Semantic change needed?}
    D -- No --> E[Return original bytes]
    D -- Yes --> F[Parse strict CST]
    F --> G[Edit Herdr-owned hook nodes]
    G --> H[Verify semantic result]
    H --> I[Write only when bytes changed]

Loading

_{Reviews (7): Last reviewed commit: "Merge branch 'master' into akbash/2066-p..." | Re-trigger Greptile}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/integration/claude_settings.rs (1)

189-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for the structural-validation error paths.

ensure_hooks_object and ensure_command_hook have distinct error branches (root not an object, hooks not an object, hook entries not an array) that aren't exercised by any test — the current tests only cover top-level JSON parse failure and success paths. A #[cfg(test)] mod tests block co-located here (testing update_claude_settings directly rather than only via install_claude) would catch regressions in these branches cheaply.

src/integration/targets.rs (1)

126-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Consider an atomic write for settings.json.

fs::write(&settings_path, updated_settings)? overwrites the file directly; an interruption mid-write (crash, kill) can leave settings.json truncated or corrupted, requiring the user to manually repair Claude's config. Writing to a temp file in the same directory and renaming over settings_path would make this update atomic.

♻️ Suggested atomic-write pattern

-    fs::write(&settings_path, updated_settings)?;
+    let tmp_path = settings_path.with_extension("json.tmp");
+    fs::write(&tmp_path, updated_settings)?;
+    fs::rename(&tmp_path, &settings_path)?;

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: dab25f02-86f1-42ed-a509-238d2aecbb56

📥 Commits

Reviewing files that changed from the base of the PR and between 9135ffe and e1b14c5.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (5)

• Cargo.toml
• src/integration/claude_settings.rs
• src/integration/mod.rs
• src/integration/targets.rs
• src/integration/tests.rs

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6cc76234-efd3-486f-b92b-58005a0a5eff

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1ab96 and e5481af.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

• Cargo.toml
• src/integration/claude_settings.rs
• src/integration/mod.rs
• src/integration/targets.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/integration/mod.rs

[coderabbitai]

🧹 Nitpick comments (4)

src/integration/claude_settings.rs (4)

280-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rename the return value to describe what it reports.

remove_event_commands returns canonical_preserved, not a removal flag. The near-identical remove_value_event_commands at line 144 returns removed. The mismatched meanings of two same-shaped functions invite misuse in future edits. Rename this function or add a doc comment that states the return meaning.

♻️ Proposed doc comment

+/// Removes Herdr-owned commands for `event` from the CST.
+///
+/// Returns `true` when the canonical `SessionStart` entry was found and
+/// preserved in place, so the caller must not append it again.
 fn remove_event_commands(
     hooks: &CstObject,
     event: &str,
     commands: &[String],
     installing: bool,
     canonical: &Value,
 ) -> io::Result<bool> {

---

515-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the compact canonical JSON from one source.

The canonical hook entry is now defined three times: canonical_hook_value at line 346, canonical_hook_input at line 357, and this hand-written string. A change to timeout, the matcher, or the command action must be applied to all three. If one drifts, verify_updated turns the mismatch into a runtime "failed to safely update claude settings" error instead of a compile error.

Build the compact string from canonical_hook_value so the values have a single owner.

♻️ Proposed consolidation

 fn canonical_hook_json(hook_path: &Path) -> io::Result<String> {
-    let command = serde_json::to_string(&hook_command(hook_path, Some("session")))?;
-    Ok(format!(
-        "{{\"matcher\":\"*\",\"hooks\":[{{\"type\":\"command\",\"command\":{command},\"timeout\":10}}]}}"
-    ))
+    // `canonical_hook_value` is the single owner of the canonical entry, so the
+    // compact form cannot drift from the semantic form used by `verify_updated`.
+    Ok(serde_json::to_string(&canonical_hook_value(hook_path))?)
 }

serde_json::to_string emits no insignificant whitespace, so the output stays compact. Because preserve_order is not enabled, the emitted key order becomes alphabetical (hooks, matcher), which changes the written bytes but not the semantics. Update the expected strings in install_keeps_compact_containers_compact accordingly, or keep the literal and add a test that asserts canonical_hook_json and canonical_hook_value parse equal.

Based on learnings, serde_json's preserve_order feature is intentionally not enabled in this repository, so Value comparison in verify_updated stays order-insensitive.

Source: Learnings

---

797-803: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct the test name to match the assertion.

The name says the content is kept unchanged. The body asserts install returns Err. Rename the test so a future failure report describes the real contract.

♻️ Proposed rename

-    fn install_keeps_structurally_invalid_content_unchanged() {
+    fn install_rejects_structurally_invalid_content() {

---

750-781: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the uninstall no-op and event-key removal paths.

The current uninstall test covers only the partial-removal case, where a foreign command keeps the entry alive. Two uninstall branches stay untested:

• Line 111: uninstall returns the original content when no Herdr command is present. This is the byte-idempotence guarantee stated in the PR objectives.
• Line 177: hooks.remove(event) in the value path, and line 332 event_property.remove() in the CST path, run when the last entry for an event is removed. These two paths must agree, otherwise verify_updated fails.

💚 Proposed tests

#[test]
fn uninstall_is_a_byte_exact_noop_without_herdr_hooks() {
    let (settings_path, hook_path) = paths();
    let input = "{\"hooks\":{\"SessionStart\":[{\"matcher\":\"keep\",\"hooks\":[{\"type\":\"command\",\"command\":\"echo keep\"}]}]}}\r\n";

    assert_eq!(uninstall(input, settings_path, hook_path).unwrap(), input);
}

#[test]
fn uninstall_removes_the_event_key_when_the_last_entry_is_removed() {
    let (settings_path, hook_path) = paths();
    let canonical = canonical_hook_json(hook_path).unwrap();
    let input = format!("{{\"before\":1,\"hooks\":{{\"SessionStart\":[{canonical}]}},\"after\":2}}");

    let updated = uninstall(&input, settings_path, hook_path).unwrap();

    let parsed: Value = serde_json::from_str(&updated).unwrap();
    assert!(parsed["hooks"].get("SessionStart").is_none());
    assert_eq!(parsed["before"], 1);
    assert_eq!(parsed["after"], 2);
}

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d85ccfae-7451-444e-af00-5574419154b3

📥 Commits

Reviewing files that changed from the base of the PR and between e5481af and f26cc4a.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (4)

• Cargo.toml
• src/integration/claude_settings.rs
• src/integration/mod.rs
• src/integration/targets.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• src/integration/mod.rs
• src/integration/targets.rs
• Cargo.toml