From 81a741164b59972ad7308b4a10085c1d34b9ac27 Mon Sep 17 00:00:00 2001 From: John Morrissey <544926+tachyon-beep@users.noreply.github.com> Date: Sat, 30 May 2026 13:59:33 +1000 Subject: [PATCH] fix(cli): canonicalise SessionStart hooks + harden migration/install tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups from the multi-agent review of PR #21 (merged). Three agents converged on merge_session_start_hook under-delivering its docstring. Behavior (review #1/#10): - merge_session_start_hook now canonicalises Clarion-owned hooks to exactly one running the desired command: refresh the first, remove any extras (a stale duplicate, or one pinned to a different project), and drop a Clarion-dedicated group left empty. Previously it short-circuited on a current entry and refreshed only the first stale one, so a current hook coexisting with a wrong-project pin (hand-merged settings) silently oriented the wrong project every session, and multiple stale entries left survivors. Idempotency preserved (single exact match → no-op). Tests: different-path refresh, current+stale removal, multi-stale dedup. Tests / docs: - schema.rs: migration_0002_records_its_own_version_row exercises 0002's SQL directly (bypassing apply_one's INSERT OR IGNORE fallback) so the in-transaction version-row INSERT is load-bearing — verified it fails if that line is dropped. The rollback test now also asserts the version-2 row rolls back together with the column. (review #2) - install.rs: correct the non-dir-guard comment — only --all reaches that block (--skills/--hooks alone are Components with init_clarion()==false). (review #3) - install.rs tests: cover the --force non-directory .clarion guard. (review #7) - hooks_settings.rs: shell_single_quote("") -> "''" edge case. (review #6) Co-Authored-By: Claude Opus 4.8 --- crates/clarion-cli/src/hooks_settings.rs | 188 +++++++++++++++++------ crates/clarion-cli/src/install.rs | 11 +- crates/clarion-cli/tests/install.rs | 21 +++ crates/clarion-storage/src/schema.rs | 39 +++++ 4 files changed, 212 insertions(+), 47 deletions(-) diff --git a/crates/clarion-cli/src/hooks_settings.rs b/crates/clarion-cli/src/hooks_settings.rs index f9ec62a3..689bfdda 100644 --- a/crates/clarion-cli/src/hooks_settings.rs +++ b/crates/clarion-cli/src/hooks_settings.rs @@ -1,11 +1,11 @@ //! `.claude/settings.json` SessionStart-hook merge. //! -//! Merge semantics (never clobber): parse existing JSON and ensure a +//! Merge semantics (never clobber): parse existing JSON and ensure exactly one //! `SessionStart` matcher-group runs `clarion hook session-start --path -//! ` (the project path POSIX-single-quote-escaped for the shell). If a -//! Clarion-owned hook already runs the exact command, it is left untouched; a -//! stale one (the old path-less form, or one pinned to a different project) is -//! refreshed in place. Every other key is preserved. +//! ` (the project path POSIX-single-quote-escaped for the shell). +//! Clarion-owned hooks are canonicalised — the first is refreshed to the +//! desired command and any extras (a stale duplicate, or one pinned to a +//! different project) are removed. Every other key is preserved. //! //! Verified against the Claude Code settings schema: `hooks.SessionStart` is an //! array of matcher-groups, each `{ "matcher"?, "hooks": [ {type,command} ] }`. @@ -44,12 +44,13 @@ fn shell_single_quote(s: &str) -> String { /// inserting the supplied `command` (which must contain [`HOOK_COMMAND`] so it /// is recognised as Clarion-owned). Returns `true` if a change was made. /// -/// Clarion-owned entries are keyed on the [`HOOK_COMMAND`] substring. If one -/// already runs the exact `command`, this is a no-op (`false`). If a Clarion -/// hook exists but differs (the old path-less form, or one pinned to a -/// different project), it is refreshed in place to `command` (`true`) rather -/// than no-oping forever or appending a duplicate. If none exists, the hook is -/// appended (`true`). +/// Clarion-owned entries are keyed on the [`HOOK_COMMAND`] substring. The merge +/// canonicalises them to exactly one hook running `command`: the first is +/// refreshed and any extras (a stale duplicate, or a hook pinned to a different +/// project — possible in hand-merged settings) are removed, dropping any +/// Clarion-dedicated group left empty. If none exists, the hook is appended. +/// Returns `false` only when a single Clarion hook already runs `command` (the +/// idempotent re-install case); otherwise `true`. #[must_use] pub fn merge_session_start_hook(settings: &mut Value, command: &str) -> bool { // Coercion-after-parse: a successfully-parsed but malformed shape (a wrong @@ -90,49 +91,72 @@ pub fn merge_session_start_hook(settings: &mut Value, command: &str) -> bool { ); } - // Classify existing Clarion-owned hooks (command contains HOOK_COMMAND). - // If one already equals `command`, we're current — no-op. Otherwise, if a - // stale Clarion hook exists (the old path-less form, or one pinned to a - // different project), refresh it in place rather than no-oping forever or - // appending a duplicate. Two passes keep the borrow checker happy and avoid - // mutating before we know whether a current entry exists elsewhere. - let mut found_current = false; - let mut stale_loc: Option<(usize, usize)> = None; + // Locate every Clarion-owned hook (its command contains HOOK_COMMAND), + // across all matcher-groups. Pass 1 only reads, so the immutable borrow is + // released before any mutation below. + let mut locations: Vec<(usize, usize)> = Vec::new(); for (gi, group) in groups.iter().enumerate() { let Some(inner) = group.get("hooks").and_then(Value::as_array) else { continue; }; for (hi, h) in inner.iter().enumerate() { - let Some(c) = h.get("command").and_then(Value::as_str) else { - continue; - }; - if !c.contains(HOOK_COMMAND) { - continue; - } - if c == command { - found_current = true; - } else if stale_loc.is_none() { - stale_loc = Some((gi, hi)); + if h.get("command") + .and_then(Value::as_str) + .is_some_and(|c| c.contains(HOOK_COMMAND)) + { + locations.push((gi, hi)); } } } - if found_current { - return false; - } - if let Some((gi, hi)) = stale_loc { - groups[gi]["hooks"][hi]["command"] = Value::String(command.to_string()); + + if locations.is_empty() { + groups.push(json!({ + "hooks": [ + { + "type": "command", + "command": command + } + ] + })); return true; } - groups.push(json!({ - "hooks": [ - { - "type": "command", - "command": command - } - ] - })); - true + // Canonicalise to exactly one Clarion hook running `command`: refresh the + // first, then remove any extras (a stale duplicate, or a hook pinned to a + // different project — e.g. hand-merged settings). This delivers "don't + // no-op on a stale hook, don't leave duplicates, don't silently keep a + // wrong-project pin" even when a current and a stale entry coexist or + // multiple stale entries exist. Returns `false` only when a single Clarion + // hook already runs `command` (the idempotent re-install case). + let mut changed = false; + let (kg, kh) = locations[0]; + if groups[kg]["hooks"][kh]["command"].as_str() != Some(command) { + groups[kg]["hooks"][kh]["command"] = Value::String(command.to_string()); + changed = true; + } + + // Remove the extras. Descending order keeps inner indices valid as we go. + let mut extras: Vec<(usize, usize)> = locations[1..].to_vec(); + extras.sort_unstable_by(|a, b| b.cmp(a)); + let mut touched_groups = std::collections::BTreeSet::new(); + for (gi, hi) in extras { + if let Some(inner) = groups[gi]["hooks"].as_array_mut() { + inner.remove(hi); + touched_groups.insert(gi); + changed = true; + } + } + // Drop any Clarion-dedicated group we just emptied (descending to keep + // indices valid). A group still holding unrelated hooks is left intact. + for gi in touched_groups.into_iter().rev() { + if groups[gi]["hooks"] + .as_array() + .is_some_and(std::vec::Vec::is_empty) + { + groups.remove(gi); + } + } + changed } /// Read `.claude/settings.json` under `project_root` (creating an empty object @@ -273,6 +297,7 @@ mod tests { "/back\\slash/x", "/dquote\"here/x", "/a'b\"c$d`e/x", + "", // the one structurally distinct (zero-iteration) input ] { assert_eq!( sh_roundtrip(&super::shell_single_quote(s)), @@ -280,6 +305,8 @@ mod tests { "shell_single_quote did not round-trip {s:?} through sh" ); } + // The empty string must produce a valid empty shell word, not nothing. + assert_eq!(super::shell_single_quote(""), "''"); } #[test] @@ -338,6 +365,81 @@ mod tests { ); } + /// All `command` strings across every `SessionStart` group that look like a + /// Clarion-owned hook (contain [`HOOK_COMMAND`]). + fn clarion_commands(settings: &serde_json::Value) -> Vec { + settings["hooks"]["SessionStart"] + .as_array() + .into_iter() + .flatten() + .filter_map(|g| g["hooks"].as_array()) + .flatten() + .filter_map(|h| h["command"].as_str()) + .filter(|c| c.contains(HOOK_COMMAND)) + .map(str::to_owned) + .collect() + } + + #[test] + fn refreshes_a_stale_hook_pinned_to_a_different_path() { + // The realistic re-install case: the repo moved, so the existing hook + // pins the old project path. It must be refreshed to the new path, not + // no-oped. (clarion review #4/#10) + let mut settings = json!({ + "hooks": {"SessionStart": [ + {"hooks": [{"type": "command", + "command": "clarion hook session-start --path '/old/proj'"}]} + ]} + }); + let desired = "clarion hook session-start --path '/new/proj'"; + assert!(merge_session_start_hook(&mut settings, desired)); + assert_eq!(clarion_commands(&settings), vec![desired.to_string()]); + } + + #[test] + fn removes_a_stale_hook_when_a_current_one_already_exists() { + // A current hook coexisting with a stale one pinned to a different + // project (e.g. hand-merged settings). The stale one silently orients + // the wrong project every session, so it must be reconciled away — + // leaving exactly one Clarion hook running the desired command. + // (clarion review #10 — found_current must not short-circuit the sweep) + let desired = "clarion hook session-start --path '/proj'"; + let mut settings = json!({ + "hooks": {"SessionStart": [ + {"hooks": [{"type": "command", "command": desired}]}, + {"hooks": [{"type": "command", + "command": "clarion hook session-start --path '/other'"}]} + ]} + }); + assert!( + merge_session_start_hook(&mut settings, desired), + "a stale entry coexisting with the current one must be reconciled" + ); + assert_eq!( + clarion_commands(&settings), + vec![desired.to_string()], + "exactly one Clarion hook must remain, running the desired command" + ); + } + + #[test] + fn dedups_multiple_stale_clarion_hooks() { + // Two stale Clarion hooks, no current one. Must converge to a single + // hook running the desired command, not leave survivors. (clarion #10) + let desired = "clarion hook session-start --path '/proj'"; + let mut settings = json!({ + "hooks": {"SessionStart": [ + {"hooks": [{"type": "command", "command": "clarion hook session-start"}]}, + {"hooks": [{"type": "command", + "command": "clarion hook session-start --path '/old'"}]} + ]} + }); + assert!(merge_session_start_hook(&mut settings, desired)); + assert_eq!(clarion_commands(&settings), vec![desired.to_string()]); + // Convergent: a second merge is now a no-op. + assert!(!merge_session_start_hook(&mut settings, desired)); + } + #[test] fn preserves_unrelated_hooks_and_top_level_keys() { let mut settings = json!({ diff --git a/crates/clarion-cli/src/install.rs b/crates/clarion-cli/src/install.rs index 352429d2..9a66b6e1 100644 --- a/crates/clarion-cli/src/install.rs +++ b/crates/clarion-cli/src/install.rs @@ -201,10 +201,13 @@ pub fn run(path: &Path, force: bool, plan: InstallPlan) -> Result<()> { clarion_dir.display() ); } - // A non-bare init (--all / --skills / --hooks) treats an existing - // .clarion/ as already-initialised and keeps it. But a non-directory - // .clarion is not a usable index — refuse rather than "succeed" with - // skills/hooks installed atop a project that has no clarion.db. + // Only `--all` reaches here: a bare install bailed above, and + // `--skills`/`--hooks` alone are `Components` (init_clarion() == + // false) that skip this whole block. `--all` treats an existing + // .clarion/ as already-initialised and keeps it — but a + // non-directory .clarion is not a usable index, so refuse rather + // than "succeed" with skills/hooks atop a project that has no + // clarion.db. if !clarion_dir.is_dir() { bail!( "found a non-directory at {}; expected an initialised .clarion/ \ diff --git a/crates/clarion-cli/tests/install.rs b/crates/clarion-cli/tests/install.rs index 4befc41a..40bc4f8c 100644 --- a/crates/clarion-cli/tests/install.rs +++ b/crates/clarion-cli/tests/install.rs @@ -97,6 +97,27 @@ fn install_all_rejects_non_directory_clarion() { ); } +#[test] +fn install_force_rejects_non_directory_clarion() { + // The --force overwrite path has its own non-directory guard (distinct from + // the --all skip-init guard): it can only remove an existing .clarion/ + // *directory*, never a regular file masquerading as one. + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join(".clarion"), "i am a file, not a dir").unwrap(); + + let out = clarion_bin() + .args(["install", "--force", "--path"]) + .arg(dir.path()) + .env("PATH", "") + .assert() + .failure(); + let stderr = String::from_utf8(out.get_output().stderr.clone()).unwrap(); + assert!( + stderr.contains("can only overwrite an existing .clarion/ directory"), + "error did not mention the --force non-directory guard: {stderr}" + ); +} + #[test] fn install_refuses_to_overwrite_existing_clarion_dir() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/clarion-storage/src/schema.rs b/crates/clarion-storage/src/schema.rs index 1ade8f5f..c9510138 100644 --- a/crates/clarion-storage/src/schema.rs +++ b/crates/clarion-storage/src/schema.rs @@ -260,6 +260,45 @@ mod tests { "0002's ALTER must roll back when a later statement fails; \ the column survived, so the migration is not atomic" ); + // The version-2 row must roll back *with* the column — proving the two + // commit together. If the column rolled back but a version row had been + // recorded, the next upgrade would skip 0002 forever. + let v2_present: bool = conn + .query_row( + "SELECT EXISTS(SELECT 1 FROM schema_migrations WHERE version = 2)", + [], + |row| row.get(0), + ) + .unwrap(); + assert!( + !v2_present, + "the version-2 row must roll back together with the column" + ); + } + + #[test] + fn migration_0002_records_its_own_version_row() { + // apply_one writes an INSERT OR IGNORE version row as a fallback, which + // would mask a migration that forgot to record itself. Exercise 0002's + // SQL directly (bypassing apply_one) so the in-transaction + // `INSERT INTO schema_migrations` is load-bearing: this fails if that + // line is dropped, reopening the duplicate-column upgrade bug. + let mut conn = Connection::open_in_memory().unwrap(); + apply_one(&mut conn, &MIGRATIONS[0]).expect("apply initial migration"); + conn.execute_batch(MIGRATIONS[1].sql) + .expect("apply 0002 SQL directly"); + let recorded: bool = conn + .query_row( + "SELECT EXISTS(SELECT 1 FROM schema_migrations \ + WHERE version = 2 AND name = '0002_briefing_blocked')", + [], + |row| row.get(0), + ) + .unwrap(); + assert!( + recorded, + "migration 0002 must record its own schema_migrations row inside its transaction" + ); } #[test]