fix: preserve claude settings key order - #2089
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClaude 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. ChangesClaude settings migration
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
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
Greptile SummaryPreserves Claude settings formatting while migrating Herdr-managed hooks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| 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 | Delegates Claude settings installation and removal to the lossless editor and skips byte-identical writes. |
| src/integration/mod.rs | Registers the new Claude settings integration module. |
| Cargo.toml | Adds jsonc-parser with only the CST and Serde JSON features enabled. |
| Cargo.lock | Locks jsonc-parser 0.33.1 and its existing Serde JSON dependency. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Read Claude settings bytes] --> B[Parse semantic JSON]
B --> C[Compute desired hook state]
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[Parse edited result]
H --> I{Matches desired semantics?}
I -- Yes --> J[Write only if bytes changed]
I -- No --> K[Return safety error]
Reviews (5): Last reviewed commit: "fix: preserve claude settings formatting" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/integration/claude_settings.rs (1)
189-261: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the structural-validation error paths.
ensure_hooks_objectandensure_command_hookhave distinct error branches (root not an object,hooksnot 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 testsblock co-located here (testingupdate_claude_settingsdirectly rather than only viainstall_claude) would catch regressions in these branches cheaply.src/integration/targets.rs (1)
126-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider an atomic write for
settings.json.
fs::write(&settings_path, updated_settings)?overwrites the file directly; an interruption mid-write (crash, kill) can leavesettings.jsontruncated or corrupted, requiring the user to manually repair Claude's config. Writing to a temp file in the same directory and renaming oversettings_pathwould 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlsrc/integration/claude_settings.rssrc/integration/mod.rssrc/integration/targets.rssrc/integration/tests.rs
02ca4a6 to
5b1ab96
Compare
5b1ab96 to
e5481af
Compare
There was a problem hiding this comment.
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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/integration/claude_settings.rssrc/integration/mod.rssrc/integration/targets.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/integration/mod.rs
e5481af to
f26cc4a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/integration/claude_settings.rs (4)
280-293: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the return value to describe what it reports.
remove_event_commandsreturnscanonical_preserved, not a removal flag. The near-identicalremove_value_event_commandsat line 144 returnsremoved. 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 winDerive the compact canonical JSON from one source.
The canonical hook entry is now defined three times:
canonical_hook_valueat line 346,canonical_hook_inputat line 357, and this hand-written string. A change totimeout, the matcher, or the command action must be applied to all three. If one drifts,verify_updatedturns the mismatch into a runtime"failed to safely update claude settings"error instead of a compile error.Build the compact string from
canonical_hook_valueso 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_stringemits no insignificant whitespace, so the output stays compact. Becausepreserve_orderis not enabled, the emitted key order becomes alphabetical (hooks,matcher), which changes the written bytes but not the semantics. Update the expected strings ininstall_keeps_compact_containers_compactaccordingly, or keep the literal and add a test that assertscanonical_hook_jsonandcanonical_hook_valueparse equal.Based on learnings,
serde_json'spreserve_orderfeature is intentionally not enabled in this repository, soValuecomparison inverify_updatedstays order-insensitive.Source: Learnings
797-803: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the test name to match the assertion.
The name says the content is kept unchanged. The body asserts
installreturnsErr. 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 winAdd 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:
uninstallreturns 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 332event_property.remove()in the CST path, run when the last entry for an event is removed. These two paths must agree, otherwiseverify_updatedfails.💚 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/integration/claude_settings.rssrc/integration/mod.rssrc/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
3 participants
Summary
Checks
just checkrefs #2066