From e38c6e403532485a1500f6a05dfd5668c54e71b8 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:31:45 -0600 Subject: [PATCH 01/16] fix(core/registry): hold lockdir for full critical section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In zsh an `EXIT` trap set inside a function fires when *that* function returns, regardless of `local_traps` in the caller. Previously the trap was set inside `_core_registry_acquire_mkdir_lock`, so the lockdir was released the moment acquire returned — before the caller's jq write ever ran. A concurrent writer could grab the lock during the supposed critical section and clobber the in-flight update. Move the trap into `_core_registry_update_mkdir_fallback` (the function whose lifetime spans the critical section) and eager-expand `\$lockdir` at trap-set time, since by the time the trap actually fires the function's local is already out of scope. Add three bats tests: - acquire leaves the lockdir intact for the caller after returning; - fallback releases the lockdir after a successful update; - fallback releases the lockdir after a jq failure. Verified that the test for the regression fails on the previous implementation and passes after the fix. --- lib/core/registry.zsh | 14 ++++++++-- lib/core/registry_test.bats | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 2 deletions(-) diff --git a/lib/core/registry.zsh b/lib/core/registry.zsh index 2ab2c4b..71f0d4a 100644 --- a/lib/core/registry.zsh +++ b/lib/core/registry.zsh @@ -82,7 +82,11 @@ _core_registry_check_stale_lock() { } # Wait for the mkdir lock to become available, with stale-lock recovery. -# Sets up the EXIT trap to release the lock on success. +# The caller is responsible for releasing the lock — DO NOT install an EXIT +# trap here. In zsh, an EXIT trap set inside a function fires when *that* +# function returns, which would remove the lockdir before the caller's +# critical section runs. The trap belongs in the caller (the function whose +# lifetime spans the critical section). # # Args: # $1 — lockdir path @@ -107,7 +111,6 @@ _core_registry_acquire_mkdir_lock() { (( stale_rc == 2 )) && return 1 sleep "$LOCK_RETRY_INTERVAL_SECONDS" done - trap 'rmdir "$lockdir" 2>/dev/null' EXIT } # Perform an atomic registry update via mkdir lock (macOS fallback — no flock). @@ -123,6 +126,13 @@ _core_registry_update_mkdir_fallback() { setopt local_options local_traps local lockdir="$CKIPPER_DIR/.registry.lock.d" _core_registry_acquire_mkdir_lock "$lockdir" || return 1 + # Trap lives in this function (not in acquire) so it fires when the + # critical section is done — not when acquire returns mid-critical-section. + # Use double-quoted trap text so $lockdir is expanded NOW (at trap-set time); + # by the time the trap actually fires (after this function returns), our + # local $lockdir is out of scope, so a deferred-expansion form (single quotes) + # would expand to the empty string and rmdir would silently no-op. + trap "rmdir '$lockdir' 2>/dev/null" EXIT local registry_tmpfile; registry_tmpfile=$(mktemp "$CKIPPER_DIR/.registry.tmp.XXXXXX") if jq "$@" "$jq_filter" "$CKIPPER_REGISTRY" > "$registry_tmpfile" 2>/dev/null; then mv "$registry_tmpfile" "$CKIPPER_REGISTRY" diff --git a/lib/core/registry_test.bats b/lib/core/registry_test.bats index 12faadc..20ae488 100644 --- a/lib/core/registry_test.bats +++ b/lib/core/registry_test.bats @@ -97,3 +97,54 @@ _run_registry() { [ "$status" -ne 0 ] [[ "$output" =~ "not registered" ]] } + +# Regression: in zsh, an EXIT trap set inside a function fires when that +# function returns, regardless of `local_traps` in the caller. The previous +# implementation set the trap inside _core_registry_acquire_mkdir_lock, so +# the lockdir was removed before the caller's jq write — a concurrent writer +# could grab the lock and clobber the in-flight update. The trap must live +# in _core_registry_update_mkdir_fallback (the function that owns the +# critical section). +@test "_core_registry_acquire_mkdir_lock leaves lockdir intact for caller after acquire returns" { + # Caller mimics the production pattern in _core_registry_update_mkdir_fallback: + # declares its own `local lockdir` (so any stray trap referencing $lockdir resolves + # in caller scope) and asserts the lockdir is still present when acquire returns. + _run_registry ' + caller() { + setopt local_options local_traps + local lockdir="$CKIPPER_DIR/.registry.lock.d" + _core_registry_acquire_mkdir_lock "$lockdir" || return 1 + [[ -d "$lockdir" ]] || { + echo "BUG: lockdir was removed before caller could use it" >&2 + return 2 + } + rmdir "$lockdir" + } + caller + ' + + [ "$status" -eq 0 ] +} + +@test "_core_registry_update_mkdir_fallback releases lockdir after successful update" { + echo '{"version":1,"default":null,"accounts":{}}' > "$CKIPPER_REGISTRY" + chmod 600 "$CKIPPER_REGISTRY" + + _run_registry '_core_registry_update_mkdir_fallback ".default = \"alice\""' + + [ "$status" -eq 0 ] + [[ ! -d "$CKIPPER_DIR/.registry.lock.d" ]] + local default + default=$(jq -r '.default' "$CKIPPER_REGISTRY") + [ "$default" = "alice" ] +} + +@test "_core_registry_update_mkdir_fallback releases lockdir after jq failure" { + echo '{"version":1,"default":null,"accounts":{}}' > "$CKIPPER_REGISTRY" + chmod 600 "$CKIPPER_REGISTRY" + + _run_registry '_core_registry_update_mkdir_fallback "this is not a valid jq filter @@@"' + + [ "$status" -ne 0 ] + [[ ! -d "$CKIPPER_DIR/.registry.lock.d" ]] +} From 35715e0b07c6339584fddd5de126454887e9386b Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:36:10 -0600 Subject: [PATCH 02/16] fix(core/config): write int_array values as zsh array literals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_core_config_write_global` emitted every value as a quoted scalar regardless of schema type, so `ckipper config set ports 3000,3030,6006` rewrote the templated `CKIPPER_PORTS=(3000 3030 6006)` array literal as `CKIPPER_PORTS="3000,3030,6006"`. After the next shell startup, `CKIPPER_PORTS` was a string, so `for port in "${CKIPPER_PORTS[@]}"` in `lib/worktree/ports.zsh` iterated once with the whole CSV — silently breaking port forwarding under Docker mode. Extract `_core_config_format_line` and pick the on-disk form by schema type: int_array → `KEY=(a b c)`, everything else → `KEY="value"`. Teach `_core_config_read_global` to recognise the array form and return CSV so `ckipper config get ports` round-trips with whatever the user passed to `set`. Add four bats tests covering the regression, the round-trip, the sourcing-side shape, and idempotent rewrite of an existing array literal in place. --- lib/core/config.zsh | 47 ++++++++++++++++++++++++++++++++---- lib/core/config_test.bats | 50 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/lib/core/config.zsh b/lib/core/config.zsh index e5d3f1a..5db2dc4 100644 --- a/lib/core/config.zsh +++ b/lib/core/config.zsh @@ -25,9 +25,12 @@ _core_config_global_file() { } # Read a global value from the config file without sourcing it. +# int_array values stored as zsh array literals (KEY=(a b c)) are returned as +# CSV (a,b,c) so callers see a consistent shape regardless of on-disk form. # # Args: $1 — schema key -# Returns: 0; prints the assigned value (quotes stripped) or empty string if unset. +# Returns: 0; prints the assigned value (quotes stripped, array→CSV) or empty +# string if unset. _core_config_read_global() { local key="$1" local var @@ -38,6 +41,20 @@ _core_config_read_global() { echo "" return 0 } + local type="${_CKIPPER_SCHEMA_TYPE[$key]:-}" + if [[ "$type" == "int_array" ]]; then + awk -v v="$var" -F= ' + $1 == v { + sub(/^[^=]+=/, "") + gsub(/^\(|\)$/, "") + gsub(/^[ \t]+|[ \t]+$/, "") + gsub(/[ \t]+/, ",") + print + exit + } + ' "$file" + return 0 + fi awk -v v="$var" -F= '$1 == v { sub(/^[^=]+=/, ""); gsub(/^"|"$/, ""); print; exit }' "$file" } @@ -136,9 +153,26 @@ _core_config_reject_shell_breakout() { return 0 } +# Format a global config-file line for a given key/value pair, picking the +# right zsh syntax based on schema type. int_array values land as zsh array +# literals so a `for x in "${KEY[@]}"` consumer sees real elements; all other +# types land as quoted scalars. +# +# Args: $1 — variable name (e.g. CKIPPER_PORTS), $2 — value, $3 — schema type +# Returns: 0; prints the formatted assignment line. +_core_config_format_line() { + local var="$1" value="$2" type="$3" + if [[ "$type" == "int_array" ]]; then + echo "${var}=(${value//,/ })" + return 0 + fi + echo "${var}=\"${value}\"" +} + # Write a global key into the config file, replacing any existing assignment. # Idempotent: existing CKIPPER_= line is rewritten in place; absent keys -# are appended. +# are appended. The on-disk form depends on the schema type — see +# _core_config_format_line. # # Args: $1 — key, $2 — value # Returns: 0 on success; 1 on validation failure. @@ -147,16 +181,19 @@ _core_config_write_global() { _core_config_validate "$key" "$value" || return 1 local var var=$(_core_config_global_var "$key") + local type="${_CKIPPER_SCHEMA_TYPE[$key]:-}" + local line + line=$(_core_config_format_line "$var" "$value" "$type") local file file=$(_core_config_global_file) mkdir -p "${file:h}" [[ -f "$file" ]] || : >"$file" local tmp tmp=$(mktemp "${file}.XXXXXX") - awk -v v="$var" -v val="$value" -F= ' - $1 == v { print v "=\"" val "\""; found=1; next } + awk -v v="$var" -v repl="$line" -F= ' + $1 == v { print repl; found=1; next } { print } - END { if (!found) print v "=\"" val "\"" } + END { if (!found) print repl } ' "$file" >"$tmp" && mv "$tmp" "$file" } diff --git a/lib/core/config_test.bats b/lib/core/config_test.bats index 3e8917e..43d3d90 100644 --- a/lib/core/config_test.bats +++ b/lib/core/config_test.bats @@ -219,3 +219,53 @@ _run_config() { [ "$status" -eq 0 ] [ -f "$marker" ] } + +# Regression: _core_config_write_global emitted `CKIPPER_PORTS="3000,3030,6006"` +# regardless of schema type, so an int_array key got rewritten as a quoted scalar. +# When the file was sourced, CKIPPER_PORTS became a string, breaking +# `for port in "${CKIPPER_PORTS[@]}"` in lib/worktree/ports.zsh. The writer must +# emit zsh array literal form for int_array types so the value round-trips +# correctly through both the file and the reader. + +@test "_core_config_set writes int_array as zsh array literal (not quoted scalar)" { + _run_config "_core_config_set ports 3000,3030,6006" + + [ "$status" -eq 0 ] + run grep -E '^CKIPPER_PORTS=' "$CKIPPER_DIR/docker/ckipper-config.zsh" + [ "$status" -eq 0 ] + [[ "$output" == 'CKIPPER_PORTS=(3000 3030 6006)' ]] +} + +@test "_core_config_get round-trips an int_array as CSV" { + _run_config "_core_config_set ports 3000,3030,6006 && _core_config_get ports" + + [ "$status" -eq 0 ] + [ "$output" = "3000,3030,6006" ] +} + +@test "sourcing the written config file populates CKIPPER_PORTS as a real array" { + _run_config ' + _core_config_set ports 3000,3030,6006 || exit 1 + unset CKIPPER_PORTS + source "$CKIPPER_DIR/docker/ckipper-config.zsh" + # Assert array shape: 3 elements, first is 3000. + (( ${#CKIPPER_PORTS[@]} == 3 )) || { echo "expected 3 elements, got ${#CKIPPER_PORTS[@]}" >&2; exit 2; } + [[ "${CKIPPER_PORTS[1]}" == "3000" ]] || { echo "expected first=3000, got ${CKIPPER_PORTS[1]}" >&2; exit 3; } + [[ "${CKIPPER_PORTS[3]}" == "6006" ]] || { echo "expected third=6006, got ${CKIPPER_PORTS[3]}" >&2; exit 4; } + ' + + [ "$status" -eq 0 ] +} + +@test "_core_config_set rewrites a pre-existing array literal in place (idempotent)" { + echo 'CKIPPER_PORTS=(3000)' > "$CKIPPER_DIR/docker/ckipper-config.zsh" + + _run_config "_core_config_set ports 4000,5000" + + [ "$status" -eq 0 ] + local count + count=$(grep -c '^CKIPPER_PORTS=' "$CKIPPER_DIR/docker/ckipper-config.zsh") + [ "$count" = "1" ] + run grep -E '^CKIPPER_PORTS=' "$CKIPPER_DIR/docker/ckipper-config.zsh" + [[ "$output" == 'CKIPPER_PORTS=(4000 5000)' ]] +} From 71a4ce9db61542cdb609403e43bea215288abbb3 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:39:43 -0600 Subject: [PATCH 03/16] fix(account): switch keychain picker to stdout-capture (zsh has no nameref) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `local -n _picked_ref="$2"` is invalid in zsh — the `local` builtin errored silently and the subsequent `_picked_ref="$picked"` leaked the value to a literal global named `_picked_ref` instead of the caller's variable. The adopt flow on macOS therefore never honoured the user's Keychain selection: `picked` stayed empty, so the registry recorded the account with no associated Keychain service. Drop the failed nameref pattern. Echo the chosen service to stdout, make the caller capture it: `picked=$(_helper "$name") || return 1`. This matches the rest of the module's style (snapshot, validate, etc. all use stdout-capture). Add four bats tests: stdout shape on valid pick, empty output on skip, empty output when the keychain has no candidates, and the existing "invalid service shape" error path. --- lib/account/account-management.zsh | 18 ++++---- lib/account/account-management_test.bats | 52 ++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/lib/account/account-management.zsh b/lib/account/account-management.zsh index 4b0cf6f..254ec6e 100644 --- a/lib/account/account-management.zsh +++ b/lib/account/account-management.zsh @@ -52,7 +52,7 @@ _ckipper_account_add_adopt_flow() { fi local picked="" if [[ "${_CKIPPER_TEST_OSTYPE:-$OSTYPE}" == darwin* ]]; then - _ckipper_account_add_pick_keychain_entry "$name" picked || return 1 + picked=$(_ckipper_account_add_pick_keychain_entry "$name") || return 1 fi _CKIPPER_FINALIZE_CTX[name]="$name" _CKIPPER_FINALIZE_CTX[dir]="$dir" @@ -66,18 +66,22 @@ _ckipper_account_add_adopt_flow() { readonly _CKIPPER_ACCOUNT_KEYCHAIN_SKIP_LABEL="(skip — register without Keychain entry)" # Prompt the user to pick a Keychain entry from the available candidates. -# On return, the nameref variable (arg $2) holds the chosen service (may be empty -# if the user picked the skip sentinel). +# Echoes the chosen service name to stdout (or empty string when the user +# picked the skip sentinel or no candidates exist). zsh has no working +# `local -n` / `typeset -n`, so the contract is stdout-capture rather than +# nameref — the caller does `picked=$(_ckipper_account_add_pick_keychain_entry "$name")`. # # Args: -# $1 — account name (for error messages) -# $2 — nameref variable to receive the chosen service name +# $1 — account name (for the prompt label and error messages) # # Returns: # 0 on success; 1 on keychain error or invalid service shape. +# +# Errors (stderr): +# "Invalid Keychain service shape: " — when the picked entry has +# an unexpected shape (caught by _core_keychain_validate). _ckipper_account_add_pick_keychain_entry() { local name="$1" - local -n _picked_ref="$2" local candidates candidates=$(_core_keychain_snapshot) || return 1 [[ -z "$candidates" ]] && return 0 @@ -90,7 +94,7 @@ _ckipper_account_add_pick_keychain_entry() { echo "Invalid Keychain service shape: $picked" >&2 return 1 fi - _picked_ref="$picked" + echo "$picked" } # Run the fresh registration flow: create the dir, deploy hooks, launch Claude, detect new keychain. diff --git a/lib/account/account-management_test.bats b/lib/account/account-management_test.bats index cefcfa1..bd831b0 100644 --- a/lib/account/account-management_test.bats +++ b/lib/account/account-management_test.bats @@ -219,3 +219,55 @@ run_helper() { [ "$status" -ne 0 ] [[ "$output" =~ "not registered" ]] } + +# ── _ckipper_account_add_pick_keychain_entry ───────────────────────────────── +# Regression: zsh has no working `local -n` / `typeset -n`, so the previous +# nameref-style implementation silently leaked the picked value to a global +# named `_picked_ref` and the caller's variable stayed empty. The contract is +# now stdout-capture: the function echoes the picked service to stdout (or +# nothing on skip / no candidates). + +@test "pick_keychain_entry echoes the picked service to stdout" { + run_helper ' + _core_keychain_snapshot() { printf "Claude Code-credentials\nClaude Code-credentials-personal\n"; } + _core_prompt_choose() { echo "Claude Code-credentials-personal"; } + _core_keychain_validate() { return 0; } + _ckipper_account_add_pick_keychain_entry myaccount + ' + + [ "$status" -eq 0 ] + [ "$output" = "Claude Code-credentials-personal" ] +} + +@test "pick_keychain_entry emits nothing on skip selection" { + run_helper ' + _core_keychain_snapshot() { printf "Claude Code-credentials\n"; } + _core_prompt_choose() { echo "$_CKIPPER_ACCOUNT_KEYCHAIN_SKIP_LABEL"; } + _ckipper_account_add_pick_keychain_entry myaccount + ' + + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "pick_keychain_entry emits nothing when keychain_snapshot returns no candidates" { + run_helper ' + _core_keychain_snapshot() { :; } + _ckipper_account_add_pick_keychain_entry myaccount + ' + + [ "$status" -eq 0 ] + [ -z "$output" ] +} + +@test "pick_keychain_entry returns 1 with stderr error when picked service has bad shape" { + run_helper ' + _core_keychain_snapshot() { echo "bogus-service"; } + _core_prompt_choose() { echo "bogus-service"; } + _core_keychain_validate() { return 1; } + _ckipper_account_add_pick_keychain_entry myaccount + ' + + [ "$status" -ne 0 ] + [[ "$output" =~ "Invalid Keychain service shape" ]] +} From fd542433a5fa74772ee337996a207b9e8888d76b Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:42:54 -0600 Subject: [PATCH 04/16] fix(account): refuse to remove an account while Claude is running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_ckipper_account_remove` proceeded straight into the destructive cleanup helpers — `_ckipper_account_cleanup_dir` runs `rm -rf` on the account's config directory — without ever asserting that no Claude process was alive. A user with Claude open in another terminal who ran `ckipper account remove ` would have their live session's config dir removed from underneath the running process. `account rename` already has the guard at `_ckipper_account_rename_check_preconditions:495`. `account remove` is strictly more destructive; mirror the guard right after the "is registered?" check and before any state mutation. Add a bats case that runs `_ckipper_account_remove work` with a stub pgrep reporting a fake process and `CKIPPER_FORCE=0`, and asserts non-zero exit, the registry entry preserved, and the config dir intact. --- lib/account/account-management.zsh | 8 +++-- lib/account/account-management_test.bats | 37 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/lib/account/account-management.zsh b/lib/account/account-management.zsh index 254ec6e..4503ece 100644 --- a/lib/account/account-management.zsh +++ b/lib/account/account-management.zsh @@ -416,13 +416,16 @@ _ckipper_account_default() { # Unregister an account, then prompt to delete its config dir and Keychain # entry via _ckipper_account_cleanup_*. Declining a prompt keeps the -# file/entry and prints the manual cleanup command. +# file/entry and prints the manual cleanup command. Refuses to operate while +# any Claude process is running (mirrors `account rename`) because the +# subsequent `rm -rf` of the config dir would yank state out from under a +# live session. # # Args: # $1 — account name to remove # # Returns: -# 0 on success; 1 if account is not registered. +# 0 on success; 1 if account is not registered or Claude is running. _ckipper_account_remove() { _core_registry_check_version || return 1 local name="$1" @@ -431,6 +434,7 @@ _ckipper_account_remove() { echo "Account '$name' is not registered." >&2 return 1 fi + _core_assert_no_running_claude || return 1 local dir; dir=$(jq -r --arg n "$name" '.accounts[$n].config_dir' "$CKIPPER_REGISTRY") local service; service=$(jq -r --arg n "$name" '.accounts[$n].keychain_service // ""' "$CKIPPER_REGISTRY") _core_registry_update 'del(.accounts[$n]) | (if .default == $n then .default = null else . end)' --arg n "$name" diff --git a/lib/account/account-management_test.bats b/lib/account/account-management_test.bats index bd831b0..124e7f8 100644 --- a/lib/account/account-management_test.bats +++ b/lib/account/account-management_test.bats @@ -271,3 +271,40 @@ run_helper() { [ "$status" -ne 0 ] [[ "$output" =~ "Invalid Keychain service shape" ]] } + +# ── _ckipper_account_remove ────────────────────────────────────────────────── +# Regression: `account remove` performed `rm -rf` on the account's config dir +# (via _ckipper_account_cleanup_dir) without first asserting that no Claude +# process was running. A user with Claude open in another terminal could lose +# their live session's config dir. `account rename` already had the guard; +# `remove` is strictly more destructive and now mirrors it. + +@test "account remove refuses to act when a Claude process is running" { + local dir="$TMP_HOME/.claude-work" + mkdir -p "$dir" + cat > "$CKIPPER_REGISTRY" < Date: Sun, 3 May 2026 22:45:58 -0600 Subject: [PATCH 05/16] fix(account): surface registry-write failures in default and remove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both `_ckipper_account_default` and `_ckipper_account_remove` invoked `_core_registry_update` without checking its exit status, then printed success ("Default account is now …" / "Unregistered …") regardless of whether the registry write actually landed. `account_remove` was the worse of the two: after a silent registry-write failure it still proceeded to `rm -rf` the config dir and prompt to delete the keychain entry — based on a registry state that hadn't changed. Wrap both calls in `if ! _core_registry_update …; then …` and gate the follow-up cleanup helpers on success. Surface a clear stderr error and return 1 on failure. Add bats tests that stub `_core_registry_update` to fail and assert non-zero exit, an "Error" message on stderr, the absence of the success line, and (for remove) that the config dir survives. --- lib/account/account-management.zsh | 10 ++++-- lib/account/account-management_test.bats | 45 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/lib/account/account-management.zsh b/lib/account/account-management.zsh index 4503ece..ec02ce7 100644 --- a/lib/account/account-management.zsh +++ b/lib/account/account-management.zsh @@ -410,7 +410,10 @@ _ckipper_account_default() { echo "Account '$name' is not registered." >&2 return 1 fi - _core_registry_update '.default = $n' --arg n "$name" + if ! _core_registry_update '.default = $n' --arg n "$name"; then + echo "Error: failed to set default account in registry." >&2 + return 1 + fi echo "Default account is now '$name'." } @@ -437,7 +440,10 @@ _ckipper_account_remove() { _core_assert_no_running_claude || return 1 local dir; dir=$(jq -r --arg n "$name" '.accounts[$n].config_dir' "$CKIPPER_REGISTRY") local service; service=$(jq -r --arg n "$name" '.accounts[$n].keychain_service // ""' "$CKIPPER_REGISTRY") - _core_registry_update 'del(.accounts[$n]) | (if .default == $n then .default = null else . end)' --arg n "$name" + if ! _core_registry_update 'del(.accounts[$n]) | (if .default == $n then .default = null else . end)' --arg n "$name"; then + echo "Error: failed to unregister '$name' from the registry. Skipping cleanup of '$dir'." >&2 + return 1 + fi # Drop the now-stale launcher functions from the calling shell. unset -f "claude-$name" 2>/dev/null unset -f "$name" 2>/dev/null diff --git a/lib/account/account-management_test.bats b/lib/account/account-management_test.bats index 124e7f8..ca9c7d9 100644 --- a/lib/account/account-management_test.bats +++ b/lib/account/account-management_test.bats @@ -308,3 +308,48 @@ JSON # Config dir is still present (no rm -rf happened). [[ -d "$dir" ]] } + +# ── _ckipper_account_default / remove registry-write error surfacing ───────── +# Regression: both functions called _core_registry_update without checking $?, +# then printed success ("Default account is now …" / "Unregistered …") even +# when the registry write silently failed. account_remove additionally +# proceeded to delete the config dir and keychain entry based on a registry +# state that didn't change. + +@test "account default surfaces registry-update failure and does not print success" { + cat > "$CKIPPER_REGISTRY" <<'JSON' +{"version":2,"default":null,"accounts":{"work":{"config_dir":"/tmp/.claude-work","keychain_service":null,"registered_at":"t","preferences":{}}}} +JSON + + run_helper ' + _core_registry_update() { return 1; } + _ckipper_account_default work + ' + + [ "$status" -ne 0 ] + [[ "$output" =~ "Error" ]] + [[ ! "$output" =~ "Default account is now" ]] +} + +@test "account remove surfaces registry-update failure and skips destructive cleanup" { + local dir="$TMP_HOME/.claude-work" + mkdir -p "$dir" + cat > "$CKIPPER_REGISTRY" < Date: Sun, 3 May 2026 22:51:57 -0600 Subject: [PATCH 06/16] fix(sync/settings): use JSON path arrays so hyphen/dot keys round-trip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_ckipper_account_sync_settings_*` interpolated the dotted item id straight into a jq filter string (`.cleanup-period-days`), which jq parsed as `.cleanup - .period - .days` — subtraction. Most Claude Code settings (cleanup-period-days, max-tokens, …) hit this. Result: `compare` returned "new" for both source and destination (both jq calls compile-failed silently), and `apply` aborted with a jq compile error mid-stream, rolling back the entire sync slice. A second silent-corruption shape: keys containing literal dots (rare but legal JSON, e.g. `{"some.key": …}`) were split on `.` into a multi-element path, so `setpath` planted a nested `{"some":{"key":null}}` and lost the original flat key — with rc=0. Fix the contract so the id NEVER goes through a jq filter parser: - `enumerate` emits the path as a JSON-encoded array (`["cleanup-period-days"]`, `["permissions","allow"]`); the display column keeps the dotted form for the preview UI. - `compare`, `summary`, `diff`, `apply` consume the id via `--argjson p "$id"` and use `getpath`/`setpath` — never string-interpolated jq filters, never `split(".")`. - Drop the dead `_ckipper_account_sync_settings_jq_path` helper. Update the existing settings tests to pass JSON-array ids. Add three new tests: hyphenated-key enumerate (id has correct shape), hyphenated compare (round-trips without jq compile error), hyphenated apply (value lands without aborting), and dotted-key apply (no nested-object leak). --- lib/account/sync/strategies/structured.zsh | 67 ++++++------- .../sync/strategies/structured_test.bats | 93 +++++++++++++++++-- 2 files changed, 112 insertions(+), 48 deletions(-) diff --git a/lib/account/sync/strategies/structured.zsh b/lib/account/sync/strategies/structured.zsh index 6bbe172..6291dcb 100644 --- a/lib/account/sync/strategies/structured.zsh +++ b/lib/account/sync/strategies/structured.zsh @@ -124,8 +124,14 @@ _ckipper_account_sync_mcp_apply() { # cannot do, so enumerating it here would silently plant broken absolute # paths on the destination when sync runs without `statusline` included). # +# The id column is the JSON-encoded path array (e.g. `["permissions","allow"]` +# or `["cleanup-period-days"]`). compare/diff/apply parse it back via +# `--argjson p` and use `getpath`/`setpath` — never `.foo.bar` filter strings, +# which jq would mis-parse for keys containing `-`, `.`, or other operator +# characters. The display column is the dotted string for the preview UI. +# # Args: $1 — source account dir. -# Returns: 0; prints "\t" per line. +# Returns: 0; prints "\t" per line. _ckipper_account_sync_settings_enumerate() { local src="$1" local file="$src/settings.json" @@ -138,44 +144,28 @@ _ckipper_account_sync_settings_enumerate() { | .[] | . as $p | select(($root | getpath($p)) | type != "object") - | ($p | map(tostring) | join(".")) as $k - | select($k | startswith("hooks") | not) - | select($k | startswith("statusLine") | not) - | "\($k)\t\($k)" + | ($p | map(tostring) | join(".")) as $display + | select($display | startswith("hooks") | not) + | select($display | startswith("statusLine") | not) + | "\($p | tojson)\t\($display)" ' "$file" 2>/dev/null } # Compare a jq-path between source and destination. # -# Args: $1 — src; $2 — dst; $3 — jq path (no leading dot). +# Args: $1 — src; $2 — dst; $3 — JSON path array (e.g. '["model"]'). # Returns: 0; prints "new" | "overwrite" | "unchanged". _ckipper_account_sync_settings_compare() { local src="$1" dst="$2" id="$3" - local jq_path; jq_path=$(_ckipper_account_sync_settings_jq_path "$id") local s d - s=$(jq -c "$jq_path // null" "$src/settings.json" 2>/dev/null) - d=$(jq -c "$jq_path // null" "$dst/settings.json" 2>/dev/null) + s=$(jq -c --argjson p "$id" 'getpath($p) // null' "$src/settings.json" 2>/dev/null) + d=$(jq -c --argjson p "$id" 'getpath($p) // null' "$dst/settings.json" 2>/dev/null) _ckipper_account_sync_json_status "$s" "$d" } -# Convert a dotted id like "permissions.allow" into a jq filter ".permissions.allow". -# Bare id goes to the empty filter "." which selects the document root — -# never used in practice because enumerate filters to leaves. -# -# Note: arg variable is `id` (not `path`) — zsh ties lowercase `path` to `$PATH` -# as an array, which corrupts the env if used as a local var. -# -# Args: $1 — dotted id (no leading dot). -# Returns: 0; prints jq filter string. -_ckipper_account_sync_settings_jq_path() { - local id="$1" - [[ -z "$id" ]] && { echo "."; return 0; } - echo ".$id" -} - # One-line summary for the preview table. # -# Args: $1 — src; $2 — dst; $3 — jq path. +# Args: $1 — src; $2 — dst; $3 — JSON path array. # Returns: 0; prints summary. _ckipper_account_sync_settings_summary() { local src="$1" dst="$2" id="$3" @@ -189,34 +179,31 @@ _ckipper_account_sync_settings_summary() { # Full diff for drill-down. # -# Args: $1 — src; $2 — dst; $3 — jq path. +# Args: $1 — src; $2 — dst; $3 — JSON path array. # Returns: 0; prints labeled before/after. _ckipper_account_sync_settings_diff() { local src="$1" dst="$2" id="$3" - local jq_path; jq_path=$(_ckipper_account_sync_settings_jq_path "$id") - echo "── source ($src/settings.json:$id) ──" - jq "$jq_path" "$src/settings.json" - echo "── destination ($dst/settings.json:$id) ──" - jq "$jq_path" "$dst/settings.json" 2>/dev/null + local display; display=$(jq -r -n --argjson p "$id" '$p | map(tostring) | join(".")') + echo "── source ($src/settings.json:$display) ──" + jq --argjson p "$id" 'getpath($p)' "$src/settings.json" + echo "── destination ($dst/settings.json:$display) ──" + jq --argjson p "$id" 'getpath($p)' "$dst/settings.json" 2>/dev/null } -# Apply: write the source value at jq-path into the destination's settings.json -# using jq's `setpath`, preserving all sibling content. Uses atomic write + -# JSON validation gate. +# Apply: write the source value at the given path into the destination's +# settings.json using jq's `setpath`, preserving all sibling content. Uses +# atomic write + JSON validation gate. # -# Args: $1 — src; $2 — dst; $3 — jq path; $4 — backup_dir. +# Args: $1 — src; $2 — dst; $3 — JSON path array; $4 — backup_dir. # Returns: 0 on success; non-zero on jq/write failure. _ckipper_account_sync_settings_apply() { local src="$1" dst="$2" id="$3" backup_dir="$4" _ckipper_account_sync_backup_file "$backup_dir" "$dst/settings.json" "settings.json" || return 1 [[ -f "$dst/settings.json" ]] || echo '{}' > "$dst/settings.json" - local jq_path; jq_path=$(_ckipper_account_sync_settings_jq_path "$id") local val_json - val_json=$(jq -c "$jq_path" "$src/settings.json") - local id_array - id_array=$(jq -c -n --arg p "$id" '$p | split(".")') + val_json=$(jq -c --argjson p "$id" 'getpath($p)' "$src/settings.json") local merged - merged=$(jq --argjson p "$id_array" --argjson v "$val_json" \ + merged=$(jq --argjson p "$id" --argjson v "$val_json" \ 'setpath($p; $v)' "$dst/settings.json") _ckipper_account_sync_json_atomic_write "$dst/settings.json" "$merged" } diff --git a/lib/account/sync/strategies/structured_test.bats b/lib/account/sync/strategies/structured_test.bats index 858f9da..7ea7e5e 100644 --- a/lib/account/sync/strategies/structured_test.bats +++ b/lib/account/sync/strategies/structured_test.bats @@ -113,20 +113,29 @@ run_in_zsh() { # ── Settings strategy ──────────────────────────────────────────────────── -@test "settings_enumerate emits top-level keys" { +@test "settings_enumerate emits top-level keys (display column is dotted)" { local src="$TMP_HOME/src" mkdir -p "$src" echo '{"env":{"FOO":"1"},"model":"opus"}' > "$src/settings.json" - run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f1 | sort | tr '\n' ','" + run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f2 | sort | tr '\n' ','" [[ "$output" == *"env.FOO"* ]] [[ "$output" == *"model"* ]] } +@test "settings_enumerate id column is JSON path array" { + local src="$TMP_HOME/src" + mkdir -p "$src" + echo '{"env":{"FOO":"1"},"model":"opus"}' > "$src/settings.json" + run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f1 | sort | tr '\n' '|'" + [[ "$output" == *'["env","FOO"]'* ]] + [[ "$output" == *'["model"]'* ]] +} + @test "settings_enumerate excludes .hooks and .statusLine (owned by other strategies)" { local src="$TMP_HOME/src" mkdir -p "$src" echo '{"statusLine":{"command":"x"},"hooks":{"PreToolUse":[]},"model":"opus"}' > "$src/settings.json" - run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f1" + run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f2" [[ "$output" != *"hooks"* ]] [[ "$output" != *"statusLine"* ]] [[ "$output" == *"model"* ]] @@ -136,16 +145,84 @@ run_in_zsh() { local src="$TMP_HOME/src" mkdir -p "$src" echo '{"permissions":{"allow":["Bash(ls:*)"],"deny":["Bash(rm:*)"]}}' > "$src/settings.json" - run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f1 | sort | tr '\n' ','" + run_in_zsh "_ckipper_account_sync_settings_enumerate '$src' | cut -f2 | sort | tr '\n' ','" [[ "$output" == *"permissions.allow,permissions.deny,"* ]] } +# Regression: hyphenated keys (most Claude Code settings — e.g. cleanup-period-days) +# previously got interpolated into a jq filter as `.cleanup-period-days`, which +# jq parsed as `.cleanup - .period - .days` (subtraction). settings_compare +# returned "new" for both source and destination (both jq calls errored to +# empty), and settings_apply aborted with a jq compile error mid-stream, +# rolling back the entire sync. +@test "settings_enumerate id is JSON-array-safe for hyphenated keys" { + local src="$TMP_HOME/src" + mkdir -p "$src" + echo '{"cleanup-period-days":7}' > "$src/settings.json" + run_in_zsh "_ckipper_account_sync_settings_enumerate '$src'" + [[ "$output" == *'["cleanup-period-days"]'* ]] + [[ "$output" == *"cleanup-period-days"* ]] +} + +@test "settings_compare handles hyphenated keys without jq compile error" { + local src="$TMP_HOME/src" dst="$TMP_HOME/dst" + mkdir -p "$src" "$dst" + echo '{"cleanup-period-days":7}' > "$src/settings.json" + echo '{"cleanup-period-days":14}' > "$dst/settings.json" + run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' '[\"cleanup-period-days\"]'" + [[ "$output" == *"overwrite"* ]] +} + +@test "settings_apply writes hyphenated keys correctly" { + local src="$TMP_HOME/src" dst="$TMP_HOME/dst" + mkdir -p "$src" "$dst" + echo '{"cleanup-period-days":7,"unrelated":"keep"}' > "$src/settings.json" + echo '{"unrelated":"keep","cleanup-period-days":14}' > "$dst/settings.json" + run_in_zsh " + backup_dir=\$(_ckipper_account_sync_backup_create '$dst' src) + _ckipper_account_sync_manifest_init \"\$backup_dir\" src dst + _ckipper_account_sync_settings_apply '$src' '$dst' '[\"cleanup-period-days\"]' \"\$backup_dir\" || echo APPLY_FAILED + jq -r '.\"cleanup-period-days\"' '$dst/settings.json' + jq -r '.unrelated' '$dst/settings.json'" + [ "$status" -eq 0 ] + [[ "$output" != *"APPLY_FAILED"* ]] + [[ "$output" == *"7"* ]] + [[ "$output" == *"keep"* ]] +} + +# Regression: keys containing literal dots (rare, but legal JSON) used to be +# silently corrupted — settings_apply did `jq -n --arg p "$id" '$p | split(".")'` +# which split "some.key" into ["some","key"] and then setpath() built a nested +# structure. The actual `"some.key"` key was clobbered with a null or replaced +# entirely. Fix: keys travel as JSON-encoded path arrays, never split. +@test "settings_apply preserves keys with literal dots" { + local src="$TMP_HOME/src" dst="$TMP_HOME/dst" + mkdir -p "$src" "$dst" + echo '{"some.key":"value-from-src"}' > "$src/settings.json" + echo '{"some.key":"value-from-dst","unrelated":"keep"}' > "$dst/settings.json" + run_in_zsh " + backup_dir=\$(_ckipper_account_sync_backup_create '$dst' src) + _ckipper_account_sync_manifest_init \"\$backup_dir\" src dst + _ckipper_account_sync_settings_apply '$src' '$dst' '[\"some.key\"]' \"\$backup_dir\" + jq -r '.\"some.key\"' '$dst/settings.json' + jq -r '.unrelated' '$dst/settings.json' + if jq -e 'has(\"some\") and (.some | type == \"object\")' '$dst/settings.json' >/dev/null 2>&1; then + echo NESTED_OBJECT_LEAKED + else + echo NO_NESTED_LEAK + fi" + [ "$status" -eq 0 ] + [[ "$output" == *"value-from-src"* ]] + [[ "$output" == *"keep"* ]] + [[ "$output" == *"NO_NESTED_LEAK"* ]] +} + @test "settings_compare: new when path missing in destination" { local src="$TMP_HOME/src" dst="$TMP_HOME/dst" mkdir -p "$src" "$dst" echo '{"model":"opus"}' > "$src/settings.json" echo '{}' > "$dst/settings.json" - run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' model" + run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' '[\"model\"]'" [[ "$output" == *"new"* ]] } @@ -154,7 +231,7 @@ run_in_zsh() { mkdir -p "$src" "$dst" echo '{"model":"opus"}' > "$src/settings.json" echo '{"model":"opus"}' > "$dst/settings.json" - run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' model" + run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' '[\"model\"]'" [[ "$output" == *"unchanged"* ]] } @@ -162,7 +239,7 @@ run_in_zsh() { local src="$TMP_HOME/src" dst="$TMP_HOME/dst" mkdir -p "$src" "$dst" echo '{"model":"opus"}' > "$src/settings.json" - run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' model" + run_in_zsh "_ckipper_account_sync_settings_compare '$src' '$dst' '[\"model\"]'" [[ "$output" == *"new"* ]] [[ "$output" != *"overwrite"* ]] } @@ -175,7 +252,7 @@ run_in_zsh() { run_in_zsh " backup_dir=\$(_ckipper_account_sync_backup_create '$dst' src) _ckipper_account_sync_manifest_init \"\$backup_dir\" src dst - _ckipper_account_sync_settings_apply '$src' '$dst' 'permissions.allow' \"\$backup_dir\" + _ckipper_account_sync_settings_apply '$src' '$dst' '[\"permissions\",\"allow\"]' \"\$backup_dir\" jq -c '.permissions.allow' '$dst/settings.json' jq -c '.permissions.deny' '$dst/settings.json' jq -r '.unrelated' '$dst/settings.json'" From b53ff8cd162afee249ebc4392d7c2df496ae3af3 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:54:49 -0600 Subject: [PATCH 07/16] fix(hooks): close rm -r bypass and stop self-blocking --force-with-lease MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regex bugs in the bash-guardrails safety hook: 1. The recursive-delete guard required *both* `r` AND `f` in the rm flags (regex alternatives `r[a-zA-Z]*f` or `f[a-zA-Z]*r` or `--recursive`). Plain `rm -r /home/user/important` matched none of them and was silently allowed. With `--dangerously-skip-permissions` this could erase arbitrary user state. Replace with a single alternation that requires *either* an r/R flag or `--recursive`. 2. The git-history guard used `--force\b`. `\b` fires at any word/non-word boundary, including the `-` in `--force-with-lease` — the very replacement the error message tells the user to switch to. Anchor the right side against whitespace-or-end-of-string instead. Add seven new bats cases: rm -r blocked, rm -R blocked, rm --recursive blocked, rm -rf node_modules still allowed, --force-with-lease passes, --force still blocked, -f still blocked. --- hooks/bash-guardrails.sh | 11 +++++-- hooks/bash-guardrails_test.bats | 57 +++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/hooks/bash-guardrails.sh b/hooks/bash-guardrails.sh index ae3f474..0ae4cbd 100755 --- a/hooks/bash-guardrails.sh +++ b/hooks/bash-guardrails.sh @@ -25,7 +25,11 @@ CMD=$(echo "$INPUT" | jq -r '.tool_input.command // empty') || { NORMALIZED=$(echo "$CMD" | sed 's/^[[:space:]]*sudo[[:space:]]*//' | tr -s ' ') # 1. Destructive recursive deletes (allow only build artifacts) -if echo "$NORMALIZED" | grep -qE 'rm\s+(-[a-zA-Z]*r[a-zA-Z]*f|--recursive|-[a-zA-Z]*f[a-zA-Z]*r)\s'; then +# Match any short-flag block containing r/R (so `rm -r`, `rm -R`, `rm -rf`, +# `rm -fr`, `rm -RfX` all match) OR the long form `--recursive`. Earlier +# revisions required BOTH r AND f, which let plain `rm -r /home/user/foo` +# bypass the check entirely. +if echo "$NORMALIZED" | grep -qE 'rm\s+(-[a-zA-Z]*[rR][a-zA-Z]*|--recursive)\s'; then SAFE="node_modules|dist|\.next|build|\.cache|__pycache__|\.turbo|coverage|\.pytest_cache|tmp|\.parcel-cache|out" if ! echo "$NORMALIZED" | grep -qE "rm\s+-[^ ]+\s+\.?/?(${SAFE})(/|\s|$)"; then echo "Blocked: recursive delete. Only build artifacts (node_modules, dist, .next, etc.) can be rm -rf'd." >&2 @@ -34,7 +38,10 @@ if echo "$NORMALIZED" | grep -qE 'rm\s+(-[a-zA-Z]*r[a-zA-Z]*f|--recursive|-[a-zA fi # 2. Git history destruction -if echo "$NORMALIZED" | grep -qE 'git\s+push\s+.*--force\b|git\s+push\s+-f\b'; then +# Anchor `--force` against whitespace-or-end-of-string so the recommended +# replacement `--force-with-lease` (the `-` is non-whitespace) is not also +# matched by `--force\b` — `\b` fires at the word/non-word boundary. +if echo "$NORMALIZED" | grep -qE 'git\s+push\s+.*--force(\s|$)|git\s+push\s+-f(\s|$)'; then echo "Blocked: git push --force. Use --force-with-lease instead." >&2 exit 2 fi diff --git a/hooks/bash-guardrails_test.bats b/hooks/bash-guardrails_test.bats index 8477eab..c7f3659 100644 --- a/hooks/bash-guardrails_test.bats +++ b/hooks/bash-guardrails_test.bats @@ -42,3 +42,60 @@ _run_guardrails() { [ "$status" -eq 2 ] [[ "$output" =~ "Error" || "$output" =~ "error" || "$output" =~ "JSON" ]] } + +# Regression: the rm-recursive guard required BOTH `r` AND `f` flags +# (regex `r[a-zA-Z]*f` or `f[a-zA-Z]*r`), so `rm -r /home/user/important` +# bypassed the check. With --dangerously-skip-permissions this could wipe +# arbitrary user state. Now the guard requires *either* recursive flag. +@test "bash-guardrails blocks rm -r without -f on a user path" { + _run_guardrails "rm -r /home/user/important-files" + + [ "$status" -eq 2 ] + [[ "$output" =~ "Blocked" ]] +} + +@test "bash-guardrails blocks rm -R (capital recursive flag)" { + _run_guardrails "rm -R /home/user/important-files" + + [ "$status" -eq 2 ] + [[ "$output" =~ "Blocked" ]] +} + +@test "bash-guardrails blocks rm --recursive on a user path" { + _run_guardrails "rm --recursive /home/user/important-files" + + [ "$status" -eq 2 ] + [[ "$output" =~ "Blocked" ]] +} + +@test "bash-guardrails still allows rm -rf on a build-artifact dir" { + _run_guardrails "rm -rf node_modules" + + [ "$status" -eq 0 ] +} + +# Regression: `\b` is a word/non-word boundary. In `--force-with-lease`, +# `force` is followed by `-` (non-word), so `\b` fired and the previous +# regex `git\s+push\s+.*--force\b` matched `--force-with-lease` — the +# very replacement the error message tells the user to switch to. Now +# the right side is anchored against whitespace or end-of-string, so +# `--force-with-lease` passes through. +@test "bash-guardrails allows git push --force-with-lease (the recommended replacement)" { + _run_guardrails "git push origin main --force-with-lease" + + [ "$status" -eq 0 ] +} + +@test "bash-guardrails still blocks git push --force" { + _run_guardrails "git push origin main --force" + + [ "$status" -eq 2 ] + [[ "$output" =~ "force" ]] +} + +@test "bash-guardrails still blocks git push -f" { + _run_guardrails "git push -f origin main" + + [ "$status" -eq 2 ] + [[ "$output" =~ "force" ]] +} From 2e00045219923cfd27ba42c5933929eb7949bfc6 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 22:58:53 -0600 Subject: [PATCH 08/16] refactor(core): move schema.zsh into lib/core/ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema arrays (`_CKIPPER_SCHEMA_TYPE`, `_DEFAULT`, `_SCOPE`, `_DESCRIPTION`) are pure data — no functions, no behavior. They were sourced from `lib/config/schema.zsh` but read directly by foundation modules (`lib/core/config.zsh`, `lib/core/registry.zsh`), which violated the dependency-direction rule from `.claude/rules/file-organization.md`: "Parent directories MUST NOT import from child route/feature directories." Move the file to `lib/core/schema.zsh` (still 9 source files in `lib/core/`, under the 10-file cap) and chase every `lib/config/schema` reference across: - source: ckipper.zsh, lib/account/sync/strategies/structured.zsh, lib/account/sync/registry.zsh, lib/account/doctor.zsh, lib/setup/{prompts,apply}.zsh, lib/worktree/worktree.zsh, lib/core/{config,registry}.zsh - tests: every *_test.bats file that sourced the schema - docs: README.md, CONTRIBUTING.md, doc-comments `lib/config/` still owns the user-facing get/set/unset/list/edit subcommands — only the data file moved. `make lint-merge-guards` and the full bats suite remain green (493 tests). --- CONTRIBUTING.md | 4 ++-- README.md | 2 +- ckipper.zsh | 2 +- lib/account/doctor.zsh | 4 ++-- lib/account/sync/registry.zsh | 2 +- lib/account/sync/strategies/structured.zsh | 2 +- lib/account/sync/strategies/structured_test.bats | 8 ++++---- lib/config/dispatcher_test.bats | 2 +- lib/config/edit_test.bats | 2 +- lib/config/list_test.bats | 2 +- lib/core/config.zsh | 2 +- lib/core/config_test.bats | 4 ++-- lib/core/registry.zsh | 2 +- lib/core/registry_migration_test.bats | 2 +- lib/{config => core}/schema.zsh | 0 lib/{config => core}/schema_test.bats | 6 +++--- lib/setup/apply.zsh | 2 +- lib/setup/apply_test.bats | 2 +- lib/setup/dispatcher_test.bats | 2 +- lib/setup/prompts.zsh | 2 +- lib/setup/prompts_test.bats | 2 +- lib/worktree/run_test.bats | 2 +- lib/worktree/worktree.zsh | 2 +- 23 files changed, 30 insertions(+), 30 deletions(-) rename lib/{config => core}/schema.zsh (100%) rename lib/{config => core}/schema_test.bats (91%) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7220907..26645ed 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,10 +37,10 @@ We follow the rules in [`.claude/rules/`](.claude/rules/) — please read them. ## Adding a new config key -1. Add the key to all four arrays in `lib/config/schema.zsh` — `_CKIPPER_SCHEMA_TYPE`, `_DEFAULT`, `_SCOPE`, `_DESCRIPTION`. +1. Add the key to all four arrays in `lib/core/schema.zsh` — `_CKIPPER_SCHEMA_TYPE`, `_DEFAULT`, `_SCOPE`, `_DESCRIPTION`. 2. The key is now usable via `ck config get/set/unset/list` and appears in the wizard automatically. 3. If the key affects worktree-creation behavior, update `lib/worktree/worktree.zsh` to read it via `_core_config_get`. -4. Add a test in `lib/config/schema_test.bats` to cover the new declaration. +4. Add a test in `lib/core/schema_test.bats` to cover the new declaration. ## Module structure diff --git a/README.md b/README.md index c80074a..b5e9cc6 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ cd Ckipper ## Configuration -Ckipper has a single source of truth for user-configurable settings ([`lib/config/schema.zsh`](lib/config/schema.zsh)). Use `ck config` to view and modify settings, or `ck setup` to walk the wizard. +Ckipper has a single source of truth for user-configurable settings ([`lib/core/schema.zsh`](lib/core/schema.zsh)). Use `ck config` to view and modify settings, or `ck setup` to walk the wizard. ### Global keys diff --git a/ckipper.zsh b/ckipper.zsh index 37a9d6f..b92ed56 100644 --- a/ckipper.zsh +++ b/ckipper.zsh @@ -19,7 +19,7 @@ source "$CKIPPER_REPO_DIR/lib/core/utils.zsh" source "$CKIPPER_REPO_DIR/lib/core/registry.zsh" source "$CKIPPER_REPO_DIR/lib/core/keychain.zsh" source "$CKIPPER_REPO_DIR/lib/core/fuzzy.zsh" -source "$CKIPPER_REPO_DIR/lib/config/schema.zsh" +source "$CKIPPER_REPO_DIR/lib/core/schema.zsh" source "$CKIPPER_REPO_DIR/lib/core/config.zsh" source "$CKIPPER_REPO_DIR/lib/core/style.zsh" source "$CKIPPER_REPO_DIR/lib/core/help.zsh" diff --git a/lib/account/doctor.zsh b/lib/account/doctor.zsh index b09a6a3..c5773d0 100644 --- a/lib/account/doctor.zsh +++ b/lib/account/doctor.zsh @@ -90,7 +90,7 @@ _ckipper_doctor_check_stale_w_vars() { } # Validate every CKIPPER_= assignment in ckipper-config.zsh against the -# config schema (lib/config/schema.zsh). Unknown keys produce a WARN — they +# config schema (lib/core/schema.zsh). Unknown keys produce a WARN — they # are likely typos that the source loader will silently set into a global # variable that nothing reads. # @@ -174,7 +174,7 @@ _ckipper_doctor_required_prefs_filter() { # them on next registry-touch operation; this is a heads-up, not a halt. # # The list of required keys is derived from the schema at call time so -# adding a new account-scope key in lib/config/schema.zsh updates this +# adding a new account-scope key in lib/core/schema.zsh updates this # check automatically. # # Returns: 0 always (results printed via _ckipper_doctor_check). diff --git a/lib/account/sync/registry.zsh b/lib/account/sync/registry.zsh index 6bf84d4..6c4c4fe 100644 --- a/lib/account/sync/registry.zsh +++ b/lib/account/sync/registry.zsh @@ -5,7 +5,7 @@ # syncable type is: append to all three parallel arrays AND implement the # strategy contract (see lib/account/sync/engine.zsh for the contract). # -# Mirrors the parallel-array idiom used by lib/config/schema.zsh. +# Mirrors the parallel-array idiom used by lib/core/schema.zsh. # Human-readable label, shown in pickers and the summary table. typeset -gA _CKIPPER_SYNC_TYPE_LABEL=( diff --git a/lib/account/sync/strategies/structured.zsh b/lib/account/sync/strategies/structured.zsh index 6291dcb..da1213e 100644 --- a/lib/account/sync/strategies/structured.zsh +++ b/lib/account/sync/strategies/structured.zsh @@ -214,7 +214,7 @@ _ckipper_account_sync_settings_apply() { # passes account NAMES through the dir args. This is the only strategy that # uses CKIPPER_REGISTRY rather than the dir paths. # -# Depends on lib/config/schema.zsh (account-scope key list) and +# Depends on lib/core/schema.zsh (account-scope key list) and # lib/core/config.zsh (_core_config_get/_core_config_set). # Enumerate every account-scope schema key. diff --git a/lib/account/sync/strategies/structured_test.bats b/lib/account/sync/strategies/structured_test.bats index 7ea7e5e..5f1513c 100644 --- a/lib/account/sync/strategies/structured_test.bats +++ b/lib/account/sync/strategies/structured_test.bats @@ -276,7 +276,7 @@ JSON @test "prefs_enumerate lists the 3 schema keys" { setup_prefs_registry run_in_zsh " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" _ckipper_account_sync_prefs_enumerate 'src' | cut -f1 | sort | tr '\n' ','" [[ "$output" == *"always_docker,always_firewall,ssh_forward,"* ]] } @@ -284,7 +284,7 @@ JSON @test "prefs_compare: new when destination has no override (default value)" { setup_prefs_registry run_in_zsh " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" _ckipper_account_sync_prefs_compare 'src' 'dst' always_docker" [[ "$output" == *"overwrite"* ]] @@ -293,7 +293,7 @@ JSON @test "prefs_compare: unchanged when values match" { setup_prefs_registry run_in_zsh " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" _ckipper_account_sync_prefs_compare 'src' 'dst' always_firewall" [[ "$output" == *"unchanged"* ]] @@ -302,7 +302,7 @@ JSON @test "prefs_apply writes the source value to the destination's registry entry" { setup_prefs_registry run_in_zsh " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" backup_dir=\$(_ckipper_account_sync_backup_create '$TMP_HOME/dst' src) diff --git a/lib/config/dispatcher_test.bats b/lib/config/dispatcher_test.bats index e5fd0b7..01be6fe 100644 --- a/lib/config/dispatcher_test.bats +++ b/lib/config/dispatcher_test.bats @@ -35,7 +35,7 @@ _run_config_dispatch() { CKIPPER_REGISTRY_VERSION="${CKIPPER_REGISTRY_VERSION:-2}" \ PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/core/fuzzy.zsh\" diff --git a/lib/config/edit_test.bats b/lib/config/edit_test.bats index 463a928..cd5020e 100644 --- a/lib/config/edit_test.bats +++ b/lib/config/edit_test.bats @@ -37,7 +37,7 @@ _run_config_edit() { PATH="$PATH" \ EDITOR="${EDITOR:-true}" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/config/edit.zsh\" diff --git a/lib/config/list_test.bats b/lib/config/list_test.bats index 31c95c2..21301c4 100644 --- a/lib/config/list_test.bats +++ b/lib/config/list_test.bats @@ -29,7 +29,7 @@ _run_config_list() { CKIPPER_REGISTRY_VERSION="${CKIPPER_REGISTRY_VERSION:-2}" \ PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" _core_style_header() { print -- \"## \$1\"; } diff --git a/lib/core/config.zsh b/lib/core/config.zsh index 5db2dc4..19d115f 100644 --- a/lib/core/config.zsh +++ b/lib/core/config.zsh @@ -3,7 +3,7 @@ # - global file: $CKIPPER_DIR/docker/ckipper-config.zsh (zsh assignments) # - per-account: $CKIPPER_REGISTRY (.accounts..preferences.) # -# Schema source-of-truth: lib/config/schema.zsh — must be sourced before this. +# Schema source-of-truth: lib/core/schema.zsh — must be sourced before this. # Functions here resolve the schema arrays at call time, never source-time. readonly _CKIPPER_GLOBAL_PREFIX="CKIPPER_" diff --git a/lib/core/config_test.bats b/lib/core/config_test.bats index 43d3d90..6d9dd6d 100644 --- a/lib/core/config_test.bats +++ b/lib/core/config_test.bats @@ -1,7 +1,7 @@ #!/usr/bin/env bats # Module-level tests for lib/core/config.zsh. # Verifies _core_config_get/set/unset/validate primitives against the schema -# from lib/config/schema.zsh. config.zsh is zsh-only, so each assertion spawns +# from lib/core/schema.zsh. config.zsh is zsh-only, so each assertion spawns # a zsh subshell that sources schema then config and runs the function under # test (matching the pattern in registry_test.bats and schema_test.bats). @@ -31,7 +31,7 @@ _run_config() { CKIPPER_REGISTRY="$CKIPPER_REGISTRY" \ PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" $zsh_cmd diff --git a/lib/core/registry.zsh b/lib/core/registry.zsh index 71f0d4a..240e6af 100644 --- a/lib/core/registry.zsh +++ b/lib/core/registry.zsh @@ -189,7 +189,7 @@ _core_registry_init() { # two callers cannot drift from the schema. # # Reads: _CKIPPER_SCHEMA_TYPE, _CKIPPER_SCHEMA_DEFAULT, _CKIPPER_SCHEMA_SCOPE -# (lib/config/schema.zsh — must be sourced before this is called). +# (lib/core/schema.zsh — must be sourced before this is called). # # Limitations: only handles bool, int, string, and path types. The current # schema has no account-scope `int_array` keys; if one is added, extend the diff --git a/lib/core/registry_migration_test.bats b/lib/core/registry_migration_test.bats index 49f9498..e6dcaf4 100644 --- a/lib/core/registry_migration_test.bats +++ b/lib/core/registry_migration_test.bats @@ -25,7 +25,7 @@ _run_registry() { CKIPPER_REGISTRY_VERSION="${CKIPPER_REGISTRY_VERSION:-2}" \ _CKIPPER_TEST_OSTYPE="${_CKIPPER_TEST_OSTYPE:-darwin19.0}" \ PATH="$PATH" \ - zsh -c "source \"$REPO_ROOT/lib/core/utils.zsh\"; source \"$REPO_ROOT/lib/config/schema.zsh\"; source \"$REPO_ROOT/lib/core/registry.zsh\"; $zsh_cmd" + zsh -c "source \"$REPO_ROOT/lib/core/utils.zsh\"; source \"$REPO_ROOT/lib/core/schema.zsh\"; source \"$REPO_ROOT/lib/core/registry.zsh\"; $zsh_cmd" } # Seed a v1 registry fixture with a single account. diff --git a/lib/config/schema.zsh b/lib/core/schema.zsh similarity index 100% rename from lib/config/schema.zsh rename to lib/core/schema.zsh diff --git a/lib/config/schema_test.bats b/lib/core/schema_test.bats similarity index 91% rename from lib/config/schema_test.bats rename to lib/core/schema_test.bats index b2703b0..6fca0f5 100644 --- a/lib/config/schema_test.bats +++ b/lib/core/schema_test.bats @@ -1,5 +1,5 @@ #!/usr/bin/env bats -# Module-level tests for lib/config/schema.zsh. +# Module-level tests for lib/core/schema.zsh. # Verifies the four schema arrays (TYPE, DEFAULT, SCOPE, DESCRIPTION) declare # the expected keys with the expected values. Schema is data-only zsh; bats # runs in bash, so each assertion spawns a zsh subshell that sources the file @@ -10,7 +10,7 @@ load "${BATS_TEST_DIRNAME}/../../tests/lib/test-helper.bash" # Helper: source schema.zsh in zsh and print the array entry $1[$2]. _schema_lookup() { local array_name="$1" key="$2" - run zsh -c "source \"$REPO_ROOT/lib/config/schema.zsh\"; print -- \"\${${array_name}[${key}]}\"" + run zsh -c "source \"$REPO_ROOT/lib/core/schema.zsh\"; print -- \"\${${array_name}[${key}]}\"" [ "$status" -eq 0 ] } @@ -63,7 +63,7 @@ _schema_lookup() { # Walk every key in _CKIPPER_SCHEMA_TYPE and require a non-empty # _CKIPPER_SCHEMA_DESCRIPTION entry. Any missing key is printed by name. run zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" for key in \"\${(@k)_CKIPPER_SCHEMA_TYPE}\"; do if [[ -z \"\${_CKIPPER_SCHEMA_DESCRIPTION[\$key]}\" ]]; then print -- \"missing description for \$key\" diff --git a/lib/setup/apply.zsh b/lib/setup/apply.zsh index 2b7ceea..7e202c6 100644 --- a/lib/setup/apply.zsh +++ b/lib/setup/apply.zsh @@ -5,7 +5,7 @@ # routing via _core_config_set. # # Depends on: -# - lib/config/schema.zsh (`_CKIPPER_SCHEMA_*` arrays — read by _core_config_set) +# - lib/core/schema.zsh (`_CKIPPER_SCHEMA_*` arrays — read by _core_config_set) # - lib/core/config.zsh (`_core_config_set`) # - lib/core/registry.zsh (`_core_account_dir` — account-existence guard) # diff --git a/lib/setup/apply_test.bats b/lib/setup/apply_test.bats index 12eaee1..bbc3666 100644 --- a/lib/setup/apply_test.bats +++ b/lib/setup/apply_test.bats @@ -36,7 +36,7 @@ _run_apply() { run env CKIPPER_DIR="$CKIPPER_DIR" CKIPPER_REGISTRY="$CKIPPER_REGISTRY" \ HOME="$TMP_HOME" PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/setup/apply.zsh\" diff --git a/lib/setup/dispatcher_test.bats b/lib/setup/dispatcher_test.bats index 271a05b..9f8ebd4 100644 --- a/lib/setup/dispatcher_test.bats +++ b/lib/setup/dispatcher_test.bats @@ -38,7 +38,7 @@ _run_setup() { CKIPPER_DIR="$CKIPPER_DIR" CKIPPER_REGISTRY="$CKIPPER_REGISTRY" \ HOME="$TMP_HOME" PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/utils.zsh\" source \"$REPO_ROOT/lib/core/registry.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" diff --git a/lib/setup/prompts.zsh b/lib/setup/prompts.zsh index 235647d..a6199c9 100644 --- a/lib/setup/prompts.zsh +++ b/lib/setup/prompts.zsh @@ -8,7 +8,7 @@ # `setup` is configuring. # # Depends on: -# - lib/config/schema.zsh (`_CKIPPER_SCHEMA_*` arrays) +# - lib/core/schema.zsh (`_CKIPPER_SCHEMA_*` arrays) # - lib/core/config.zsh (`_core_config_get`, `_core_config_read_global`) # - lib/core/style.zsh (`_core_style_header`, `_core_style_divider`, # `_core_style_table`) diff --git a/lib/setup/prompts_test.bats b/lib/setup/prompts_test.bats index 33c5551..81ff702 100644 --- a/lib/setup/prompts_test.bats +++ b/lib/setup/prompts_test.bats @@ -37,7 +37,7 @@ _run_prompts() { CKIPPER_DIR="$CKIPPER_DIR" CKIPPER_REGISTRY="$CKIPPER_REGISTRY" \ HOME="$TMP_HOME" PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/core/style.zsh\" source \"$REPO_ROOT/lib/core/prompt.zsh\" diff --git a/lib/worktree/run_test.bats b/lib/worktree/run_test.bats index 5e80af9..985c691 100644 --- a/lib/worktree/run_test.bats +++ b/lib/worktree/run_test.bats @@ -110,7 +110,7 @@ _run_resolve_flags() { CKIPPER_REGISTRY="$CKIPPER_REGISTRY" \ PATH="$PATH" \ zsh -c " - source \"$REPO_ROOT/lib/config/schema.zsh\" + source \"$REPO_ROOT/lib/core/schema.zsh\" source \"$REPO_ROOT/lib/core/config.zsh\" source \"$REPO_ROOT/lib/worktree/args.zsh\" source \"$REPO_ROOT/lib/worktree/run.zsh\" diff --git a/lib/worktree/worktree.zsh b/lib/worktree/worktree.zsh index 76723ec..366d1d6 100644 --- a/lib/worktree/worktree.zsh +++ b/lib/worktree/worktree.zsh @@ -195,7 +195,7 @@ _ckipper_worktree_fetch_and_create() { # records; this is the cheap, definitive answer when origin/HEAD is set. # 2. `git remote show origin` parse — slower, network-dependent fallback when # the symbolic ref isn't present locally. -# 3. $CKIPPER_DEFAULT_BRANCH — global config override (lib/config/schema.zsh +# 3. $CKIPPER_DEFAULT_BRANCH — global config override (lib/core/schema.zsh # key `default_branch`), exported by ckipper-config.zsh on shell init. # 4. Hardcoded "develop" — preserves pre-overhaul behaviour. # From 315f39f6cff10b7361850c55205b03f7d953e3d1 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:02:07 -0600 Subject: [PATCH 09/16] refactor(sync): consolidate _CKIPPER_SYNC_CTX declaration into _shared.zsh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same `typeset -gA _CKIPPER_SYNC_CTX` declaration lived in three sibling modules — engine.zsh, dispatcher.zsh, preview.zsh — each with an elaborate "re-declaration without `=()` is a no-op" comment justifying the duplication. Residue of the /simplify pass that left the parameter-object refactor half-applied: a single `=()` slip in any of the three would silently reset state set by the others. Move the canonical declaration into `_shared.zsh` (sourced first in the production load order) and drop the three redundant copies. The three module test files (engine_test.bats, dispatcher_test.bats, preview_test.bats) now also source `_shared.zsh` so the declaration is in scope when each test exercises its module in isolation. Full bats suite remains green (493 tests). --- lib/account/sync/_shared.zsh | 10 ++++++++++ lib/account/sync/dispatcher.zsh | 15 ++++----------- lib/account/sync/dispatcher_test.bats | 3 ++- lib/account/sync/engine.zsh | 9 --------- lib/account/sync/engine_test.bats | 3 ++- lib/account/sync/preview.zsh | 5 +---- lib/account/sync/preview_test.bats | 1 + 7 files changed, 20 insertions(+), 26 deletions(-) diff --git a/lib/account/sync/_shared.zsh b/lib/account/sync/_shared.zsh index 2a3b61b..7ab169d 100644 --- a/lib/account/sync/_shared.zsh +++ b/lib/account/sync/_shared.zsh @@ -5,6 +5,16 @@ # call these without a sibling import. Keep this file dependency-free # (no calls into other sync modules) so the load order stays trivial. +# Per-target context shared across the engine, dispatcher, and preview +# modules. Single canonical declaration here — those three modules must NOT +# redeclare it (any `=()` in a redeclaration would silently reset state set +# by an earlier module). Module test files source this file before the +# module under test for the same reason. +# +# Keys: src_dir, dst_dir, src_name, dst_name, backup_dir, changeset, +# summaries, items. +typeset -gA _CKIPPER_SYNC_CTX + # Compute sha256 of a file. Uses shasum (macOS- and Linux-friendly). # # Args: $1 — file path. diff --git a/lib/account/sync/dispatcher.zsh b/lib/account/sync/dispatcher.zsh index 3be224a..1ee9ef7 100644 --- a/lib/account/sync/dispatcher.zsh +++ b/lib/account/sync/dispatcher.zsh @@ -16,17 +16,10 @@ typeset -g _CKIPPER_SYNC_DRY_RUN="false" typeset -g _CKIPPER_SYNC_YES="false" typeset -g _CKIPPER_SYNC_FORCE="false" -# Per-target sync context. Populated once per target by run_one_target (and -# augmented by apply_target with backup_dir). Engine/preview/finalize helpers -# read from this map instead of taking 5–8 positional arguments, per the -# .claude/rules/code-style.md 3-parameter cap. Keys: src_dir, dst_dir, -# src_name, dst_name, backup_dir, changeset, summaries, items. -# -# Re-declaration without `=()` is a no-op so engine.zsh's matching -# declaration (sourced first in production) keeps any state set there; -# reset between sync invocations is handled by reset_args, not by the -# declaration line. -typeset -gA _CKIPPER_SYNC_CTX +# _CKIPPER_SYNC_CTX is declared in lib/account/sync/_shared.zsh — a single +# canonical declaration that the engine, dispatcher, and preview modules +# all share. Reset between invocations is handled by reset_args, not at +# declaration. # Reset all module-level _SYNC_* holders. Called at the top of every # parse_args invocation so re-running the dispatcher in the same shell diff --git a/lib/account/sync/dispatcher_test.bats b/lib/account/sync/dispatcher_test.bats index 7184b09..735df2e 100644 --- a/lib/account/sync/dispatcher_test.bats +++ b/lib/account/sync/dispatcher_test.bats @@ -8,7 +8,8 @@ teardown() { teardown_isolated_env; } run_in_zsh() { run env CKIPPER_DIR="$CKIPPER_DIR" \ - zsh -c "source \"$REPO_ROOT/lib/account/sync/registry.zsh\"; \ + zsh -c "source \"$REPO_ROOT/lib/account/sync/_shared.zsh\"; \ + source \"$REPO_ROOT/lib/account/sync/registry.zsh\"; \ source \"$REPO_ROOT/lib/account/sync/dispatcher.zsh\"; $*" } diff --git a/lib/account/sync/engine.zsh b/lib/account/sync/engine.zsh index 1329e41..4982da8 100644 --- a/lib/account/sync/engine.zsh +++ b/lib/account/sync/engine.zsh @@ -37,15 +37,6 @@ # All five functions take their arguments in the same order so the engine # can call them through _ckipper_account_sync_strategy_fn uniformly. -# Per-target context shared by apply_target → apply_one and the preview/ -# finalize helpers in dispatcher.zsh. Declared here too because engine.zsh -# is sourced before dispatcher.zsh, and engine_test.bats sources only the -# engine. Re-declaration without `=()` is a no-op so dispatcher.zsh's -# matching declaration doesn't reset state. Keys: src_dir, dst_dir, -# src_name, dst_name, backup_dir (and from dispatcher: changeset, summaries, -# items). -typeset -gA _CKIPPER_SYNC_CTX - # Compute the strategy function name for a (type, verb) pair. # # Args: $1 — type id (e.g. "mcp", "claude-md"); $2 — verb (enumerate, compare, diff --git a/lib/account/sync/engine_test.bats b/lib/account/sync/engine_test.bats index f6dc655..a660366 100644 --- a/lib/account/sync/engine_test.bats +++ b/lib/account/sync/engine_test.bats @@ -8,7 +8,8 @@ teardown() { teardown_isolated_env; } run_in_zsh() { run env CKIPPER_DIR="$CKIPPER_DIR" \ - zsh -c "source \"$REPO_ROOT/lib/account/sync/registry.zsh\"; \ + zsh -c "source \"$REPO_ROOT/lib/account/sync/_shared.zsh\"; \ + source \"$REPO_ROOT/lib/account/sync/registry.zsh\"; \ source \"$REPO_ROOT/lib/account/sync/engine.zsh\"; $*" } diff --git a/lib/account/sync/preview.zsh b/lib/account/sync/preview.zsh index 43156e7..4717159 100644 --- a/lib/account/sync/preview.zsh +++ b/lib/account/sync/preview.zsh @@ -11,10 +11,7 @@ readonly _CKIPPER_SYNC_BADGE_OVERWRITE="[~]" readonly _CKIPPER_SYNC_DIVIDER_WIDTH=45 readonly _CKIPPER_SYNC_DISPLAY_COL_WIDTH=26 -# Per-target context (declared here too because preview_test.bats sources -# only this module). See engine.zsh for the full key list. Re-declaration -# without `=()` is a no-op so we don't reset state set by earlier modules. -typeset -gA _CKIPPER_SYNC_CTX +# _CKIPPER_SYNC_CTX is declared in lib/account/sync/_shared.zsh. # Print the divider line for the summary table. # diff --git a/lib/account/sync/preview_test.bats b/lib/account/sync/preview_test.bats index ef1a65a..0fc4748 100644 --- a/lib/account/sync/preview_test.bats +++ b/lib/account/sync/preview_test.bats @@ -9,6 +9,7 @@ teardown() { teardown_isolated_env; } run_in_zsh() { run env HOME="$HOME" CKIPPER_DIR="$CKIPPER_DIR" CKIPPER_NO_GUM=1 TMP_HOME="$TMP_HOME" \ zsh -c "source \"$REPO_ROOT/lib/core/style.zsh\"; \ + source \"$REPO_ROOT/lib/account/sync/_shared.zsh\"; \ source \"$REPO_ROOT/lib/account/sync/registry.zsh\"; \ source \"$REPO_ROOT/lib/account/sync/preview.zsh\"; $*" } From a2735fce20f3be745534e5dde16e3955ef05acc1 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:05:02 -0600 Subject: [PATCH 10/16] refactor(core): namespace bare module constants with _CORE_ prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare-named globals like `KEYCHAIN_TIMEOUT_SECONDS`, `REGISTRY_FILE_PERMS`, and the various `LOCK_*` constants leaked into the user's zsh namespace and could collide with random shell vars. The 9894aaf "standardize prompt helpers and module-constant prefixes" commit moved prompt/style/fuzzy/help to the `_CORE__*` convention; keychain, registry, and config were missed. Rename in this commit: KEYCHAIN_TIMEOUT_SECONDS → _CORE_KEYCHAIN_TIMEOUT_SECONDS REGISTRY_FILE_PERMS → _CORE_REGISTRY_FILE_PERMS LOCK_NOTIFY_THRESHOLD_ATTEMPTS → _CORE_REGISTRY_LOCK_NOTIFY_THRESHOLD_ATTEMPTS LOCK_MAX_ATTEMPTS → _CORE_REGISTRY_LOCK_MAX_ATTEMPTS STALE_LOCK_AGE_THRESHOLD_SECONDS → _CORE_REGISTRY_STALE_LOCK_AGE_THRESHOLD_SECONDS LOCK_RETRY_INTERVAL_SECONDS → _CORE_REGISTRY_LOCK_RETRY_INTERVAL_SECONDS _CKIPPER_GLOBAL_PREFIX → _CORE_CONFIG_GLOBAL_PREFIX Definitions and references updated. Test files included. Bats suite remains green (493 tests). --- lib/core/config.zsh | 4 ++-- lib/core/keychain.zsh | 6 +++--- lib/core/registry.zsh | 24 ++++++++++++------------ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/lib/core/config.zsh b/lib/core/config.zsh index 19d115f..2f73441 100644 --- a/lib/core/config.zsh +++ b/lib/core/config.zsh @@ -6,7 +6,7 @@ # Schema source-of-truth: lib/core/schema.zsh — must be sourced before this. # Functions here resolve the schema arrays at call time, never source-time. -readonly _CKIPPER_GLOBAL_PREFIX="CKIPPER_" +readonly _CORE_CONFIG_GLOBAL_PREFIX="CKIPPER_" # Translate a schema key to the global file's variable name. # @@ -14,7 +14,7 @@ readonly _CKIPPER_GLOBAL_PREFIX="CKIPPER_" # Returns: 0; prints "CKIPPER_NOTIFY_BELL". _core_config_global_var() { local key="$1" - echo "${_CKIPPER_GLOBAL_PREFIX}${(U)key}" + echo "${_CORE_CONFIG_GLOBAL_PREFIX}${(U)key}" } # Path to the global config file. diff --git a/lib/core/keychain.zsh b/lib/core/keychain.zsh index 537ccc6..4ed1017 100644 --- a/lib/core/keychain.zsh +++ b/lib/core/keychain.zsh @@ -1,7 +1,7 @@ #!/usr/bin/env zsh # Shared macOS Keychain and Claude process utilities. -readonly KEYCHAIN_TIMEOUT_SECONDS=10 +readonly _CORE_KEYCHAIN_TIMEOUT_SECONDS=10 # Validate a keychain_service name before passing to `security`. # Accepts "Claude Code-credentials" optionally followed by "-". @@ -23,9 +23,9 @@ _core_keychain_validate() { # 0 always; prints the timeout command prefix to stdout (empty if none found). _core_keychain_detect_timeout_cmd() { if command -v timeout >/dev/null 2>&1; then - printf 'timeout %s' "$KEYCHAIN_TIMEOUT_SECONDS" + printf 'timeout %s' "$_CORE_KEYCHAIN_TIMEOUT_SECONDS" elif command -v gtimeout >/dev/null 2>&1; then - printf 'gtimeout %s' "$KEYCHAIN_TIMEOUT_SECONDS" + printf 'gtimeout %s' "$_CORE_KEYCHAIN_TIMEOUT_SECONDS" fi } diff --git a/lib/core/registry.zsh b/lib/core/registry.zsh index 240e6af..8d020ed 100644 --- a/lib/core/registry.zsh +++ b/lib/core/registry.zsh @@ -1,11 +1,11 @@ #!/usr/bin/env zsh # Shared registry read/write primitives for managing the ckipper accounts registry. -readonly REGISTRY_FILE_PERMS=600 -readonly LOCK_NOTIFY_THRESHOLD_ATTEMPTS=30 -readonly LOCK_MAX_ATTEMPTS=200 -readonly STALE_LOCK_AGE_THRESHOLD_SECONDS=30 -readonly LOCK_RETRY_INTERVAL_SECONDS=0.05 +readonly _CORE_REGISTRY_FILE_PERMS=600 +readonly _CORE_REGISTRY_LOCK_NOTIFY_THRESHOLD_ATTEMPTS=30 +readonly _CORE_REGISTRY_LOCK_MAX_ATTEMPTS=200 +readonly _CORE_REGISTRY_STALE_LOCK_AGE_THRESHOLD_SECONDS=30 +readonly _CORE_REGISTRY_LOCK_RETRY_INTERVAL_SECONDS=0.05 # Perform an atomic registry update via flock (Linux/GNU systems). # @@ -25,7 +25,7 @@ _core_registry_update_with_flock() { local registry_tmpfile; registry_tmpfile=$(mktemp "$CKIPPER_DIR/.registry.tmp.XXXXXX") if jq "$@" "$jq_filter" "$CKIPPER_REGISTRY" > "$registry_tmpfile" 2>/dev/null; then mv "$registry_tmpfile" "$CKIPPER_REGISTRY" - chmod "$REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" + chmod "$_CORE_REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" rc=0 else rm -f "$registry_tmpfile" @@ -68,12 +68,12 @@ _core_registry_recover_stale_lock() { # 2 if the lock is live but held too long (caller should abort). _core_registry_check_stale_lock() { local lockdir="$1" attempts="$2" - (( attempts < LOCK_MAX_ATTEMPTS )) && return 1 + (( attempts < _CORE_REGISTRY_LOCK_MAX_ATTEMPTS )) && return 1 local current_time_epoch modification_time_epoch lock_age_seconds current_time_epoch=$(date +%s) modification_time_epoch=$(_core_stat_mtime "$lockdir") lock_age_seconds=$(( current_time_epoch - ${modification_time_epoch:-$current_time_epoch} )) - if (( lock_age_seconds > STALE_LOCK_AGE_THRESHOLD_SECONDS )); then + if (( lock_age_seconds > _CORE_REGISTRY_STALE_LOCK_AGE_THRESHOLD_SECONDS )); then _core_registry_recover_stale_lock "$lockdir" "$lock_age_seconds" return 0 fi @@ -98,7 +98,7 @@ _core_registry_acquire_mkdir_lock() { local attempts=0 has_notified="false" while ! mkdir "$lockdir" 2>/dev/null; do (( attempts++ )) - if (( attempts == LOCK_NOTIFY_THRESHOLD_ATTEMPTS )) && [[ "$has_notified" = "false" ]]; then + if (( attempts == _CORE_REGISTRY_LOCK_NOTIFY_THRESHOLD_ATTEMPTS )) && [[ "$has_notified" = "false" ]]; then echo "Waiting on registry lock..." >&2 has_notified="true" fi @@ -109,7 +109,7 @@ _core_registry_acquire_mkdir_lock() { continue fi (( stale_rc == 2 )) && return 1 - sleep "$LOCK_RETRY_INTERVAL_SECONDS" + sleep "$_CORE_REGISTRY_LOCK_RETRY_INTERVAL_SECONDS" done } @@ -136,7 +136,7 @@ _core_registry_update_mkdir_fallback() { local registry_tmpfile; registry_tmpfile=$(mktemp "$CKIPPER_DIR/.registry.tmp.XXXXXX") if jq "$@" "$jq_filter" "$CKIPPER_REGISTRY" > "$registry_tmpfile" 2>/dev/null; then mv "$registry_tmpfile" "$CKIPPER_REGISTRY" - chmod "$REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" + chmod "$_CORE_REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" return 0 fi rm -f "$registry_tmpfile" @@ -180,7 +180,7 @@ _core_registry_init() { '{"version": $v, "default": null, "accounts": {}}' > "$registry_tmpfile" # mv -n (no-clobber): if another writer beat us, leave their file alone. mv -n "$registry_tmpfile" "$CKIPPER_REGISTRY" 2>/dev/null || rm -f "$registry_tmpfile" - [[ -f "$CKIPPER_REGISTRY" ]] && chmod "$REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" + [[ -f "$CKIPPER_REGISTRY" ]] && chmod "$_CORE_REGISTRY_FILE_PERMS" "$CKIPPER_REGISTRY" } # Build a JSON object of every account-scope schema key with its default From 06a312a9461b3420633416296de5e33ac63a2685 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:06:58 -0600 Subject: [PATCH 11/16] refactor(worktree): namespace bare module constants with _CKIPPER_WT_ prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same pattern as the previous core/ rename: bare-named globals leaked into the user's namespace and could collide with arbitrary shell vars. Rename: SHASUM_BITS → _CKIPPER_WT_SHASUM_BITS CLAUDE_CONTAINER_UID → _CKIPPER_WT_CLAUDE_CONTAINER_UID CLAUDE_CONTAINER_GID → _CKIPPER_WT_CLAUDE_CONTAINER_GID CREDS_TMPFS_MODE → _CKIPPER_WT_CREDS_TMPFS_MODE CREDS_TMPFS_SIZE → _CKIPPER_WT_CREDS_TMPFS_SIZE DOCKER_GROUP_ADD_HOST_ROOT → _CKIPPER_WT_DOCKER_GROUP_ADD_HOST_ROOT MAX_PORT_FALLBACK_ATTEMPTS → _CKIPPER_WT_MAX_PORT_FALLBACK_ATTEMPTS The `_CKIPPER_WT_` prefix matches the existing convention for worktree-module shared state (`CKIPPER_WT_DOCKER_ARGS`, `CKIPPER_WT_FLAG_DOCKER`, etc.); the leading underscore distinguishes "private constant" from "module-shared state". Bats remains green (493). --- lib/worktree/docker-mode.zsh | 20 ++++++++++---------- lib/worktree/ports.zsh | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/worktree/docker-mode.zsh b/lib/worktree/docker-mode.zsh index 45397a8..2d9ac39 100644 --- a/lib/worktree/docker-mode.zsh +++ b/lib/worktree/docker-mode.zsh @@ -1,19 +1,19 @@ #!/usr/bin/env zsh # Docker mode execution for `ckipper worktree run --docker`. Builds docker run args and launches the container. -readonly SHASUM_BITS=256 +readonly _CKIPPER_WT_SHASUM_BITS=256 # Container user identity. Must match useradd -u/-g in docker/Dockerfile. -readonly CLAUDE_CONTAINER_UID=1000 -readonly CLAUDE_CONTAINER_GID=1000 +readonly _CKIPPER_WT_CLAUDE_CONTAINER_UID=1000 +readonly _CKIPPER_WT_CLAUDE_CONTAINER_GID=1000 # tmpfs for /tmp/claude-creds inside the container. -readonly CREDS_TMPFS_MODE=700 -readonly CREDS_TMPFS_SIZE="1m" +readonly _CKIPPER_WT_CREDS_TMPFS_MODE=700 +readonly _CKIPPER_WT_CREDS_TMPFS_SIZE="1m" # Host gid 0 (wheel/root) added so the container user can read host-mounted files # owned by macOS staff/wheel without needing world-readable bits. -readonly DOCKER_GROUP_ADD_HOST_ROOT=0 +readonly _CKIPPER_WT_DOCKER_GROUP_ADD_HOST_ROOT=0 # Run the worktree in a Docker container. # @@ -169,8 +169,8 @@ _ckipper_worktree_docker_build_base_args() { -v "$CKIPPER_PROJECTS_DIR/$CKIPPER_WT_PROJECT/.git:$CKIPPER_PROJECTS_DIR/$CKIPPER_WT_PROJECT/.git:rw" -v "$CKIPPER_WT_ACTIVE_CONFIG_DIR:$CKIPPER_WT_ACTIVE_CONFIG_DIR:rw" -e "CLAUDE_CONFIG_DIR=$CKIPPER_WT_ACTIVE_CONFIG_DIR" - --group-add "$DOCKER_GROUP_ADD_HOST_ROOT" - --tmpfs "/tmp/claude-creds:mode=$CREDS_TMPFS_MODE,uid=$CLAUDE_CONTAINER_UID,gid=$CLAUDE_CONTAINER_GID,size=$CREDS_TMPFS_SIZE" + --group-add "$_CKIPPER_WT_DOCKER_GROUP_ADD_HOST_ROOT" + --tmpfs "/tmp/claude-creds:mode=$_CKIPPER_WT_CREDS_TMPFS_MODE,uid=$_CKIPPER_WT_CLAUDE_CONTAINER_UID,gid=$_CKIPPER_WT_CLAUDE_CONTAINER_GID,size=$_CKIPPER_WT_CREDS_TMPFS_SIZE" -v "claude-uv-cache:/home/claude/.cache/uv" -v "claude-uv-tools:/home/claude/.uv-tools" -e "UV_TOOL_DIR=/home/claude/.uv-tools/envs" @@ -265,7 +265,7 @@ _ckipper_worktree_docker_print_banner() { _ckipper_worktree_docker_snapshot_and_run() { local git_config="$CKIPPER_PROJECTS_DIR/$CKIPPER_WT_PROJECT/.git/config" local git_config_hash="" - [[ -f "$git_config" ]] && git_config_hash=$(shasum -a "$SHASUM_BITS" "$git_config" | cut -d' ' -f1) + [[ -f "$git_config" ]] && git_config_hash=$(shasum -a "$_CKIPPER_WT_SHASUM_BITS" "$git_config" | cut -d' ' -f1) local git_worktrees_dir="$CKIPPER_PROJECTS_DIR/$CKIPPER_WT_PROJECT/.git/worktrees" local -a worktrees_before=() @@ -294,7 +294,7 @@ _ckipper_worktree_docker_check_git_config_tampering() { local original_hash="$2" if [[ -n "$original_hash" && -f "$git_config" ]]; then local new_hash - new_hash=$(shasum -a "$SHASUM_BITS" "$git_config" | cut -d' ' -f1) + new_hash=$(shasum -a "$_CKIPPER_WT_SHASUM_BITS" "$git_config" | cut -d' ' -f1) if [[ "$original_hash" != "$new_hash" ]]; then echo "" echo "WARNING: .git/config was modified during the Docker session!" diff --git a/lib/worktree/ports.zsh b/lib/worktree/ports.zsh index 60e8a7e..7e03fa1 100644 --- a/lib/worktree/ports.zsh +++ b/lib/worktree/ports.zsh @@ -1,7 +1,7 @@ #!/usr/bin/env zsh # Port resolution for `ckipper worktree run --docker`. Finds available host ports for dev servers. -readonly MAX_PORT_FALLBACK_ATTEMPTS=10 +readonly _CKIPPER_WT_MAX_PORT_FALLBACK_ATTEMPTS=10 # Resolve port mappings for Docker, using fallback host ports when the # preferred port is already in use. Appends -p flags to the CKIPPER_WT_DOCKER_ARGS array. @@ -28,7 +28,7 @@ _ckipper_worktree_bind_port() { local host_port=$port local is_bound="false" - for (( i=0; i/dev/null; then _ckipper_worktree_record_bound_port "$port" "$host_port" is_bound="true" @@ -38,7 +38,7 @@ _ckipper_worktree_bind_port() { done if [[ "$is_bound" != "true" ]]; then - echo " Port $port: no available host port found ($port-$((port+MAX_PORT_FALLBACK_ATTEMPTS-1)) all in use)" + echo " Port $port: no available host port found ($port-$((port+_CKIPPER_WT_MAX_PORT_FALLBACK_ATTEMPTS-1)) all in use)" fi } From 1b096fc583dd682c96f931ecfab1b2c896926e1f Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:12:03 -0600 Subject: [PATCH 12/16] refactor(worktree): standardize boolean style and convert unquoted find loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two shell-style fixes bundled because they overlap in the worktree files: 1. Boolean comparison style: `[[ "$X" = true ]]` → `[[ "$X" = "true" ]]`. The unquoted form is non-canonical per `.claude/rules/shell-conventions.md` ("test with `[[ \"$x\" = \"true\" ]]`"). Five sites in run.zsh/docker-mode.zsh/worktree.zsh. 2. `for x in $(find ...)` loops silently lose paths containing spaces / tabs / newlines because the unquoted command substitution undergoes word-splitting on $IFS. Convert to null-delimited iteration: while IFS= read -r -d '' x; do ... done < <(find ... -print0) Four sites: ckipper.zsh:304/317 (completion), launcher/menu.zsh:96 (project enumeration), worktree.zsh:331 (.env file copy on create). Also rename `CKIPPER_WT_FIND_MAX_DEPTH` → `_CKIPPER_WT_FIND_MAX_DEPTH` (touched here anyway by the find-loop edit; the leading underscore matches the module-private-constant convention used elsewhere in worktree/). Bats remains green (493 tests). --- ckipper.zsh | 18 ++++++++++-------- lib/launcher/menu.zsh | 6 +++--- lib/worktree/docker-mode.zsh | 4 ++-- lib/worktree/run.zsh | 4 ++-- lib/worktree/worktree.zsh | 13 +++++++------ 5 files changed, 24 insertions(+), 21 deletions(-) diff --git a/ckipper.zsh b/ckipper.zsh index b92ed56..2000ee0 100644 --- a/ckipper.zsh +++ b/ckipper.zsh @@ -301,11 +301,12 @@ _ckipper() { ;; run) local -a projects - for dir in $(find "$projects_dir" -maxdepth 3 -name ".git" -type d -not -path "*/.worktrees/*" 2>/dev/null); do - local repo_dir="${dir:h}" - local rel="${repo_dir#$projects_dir/}" + local dir repo_dir rel + while IFS= read -r -d '' dir; do + repo_dir="${dir:h}" + rel="${repo_dir#$projects_dir/}" projects+=("$rel") - done + done < <(find "$projects_dir" -maxdepth 3 -name ".git" -type d -not -path "*/.worktrees/*" -print0 2>/dev/null) _describe -t projects 'project' projects && return 0 ;; esac @@ -314,11 +315,12 @@ _ckipper() { case "${words[2]}/${words[3]}" in worktree/run|wt/run|worktree/rm|wt/rm) local -a projects - for dir in $(find "$projects_dir" -maxdepth 3 -name ".git" -type d -not -path "*/.worktrees/*" 2>/dev/null); do - local repo_dir="${dir:h}" - local rel="${repo_dir#$projects_dir/}" + local dir repo_dir rel + while IFS= read -r -d '' dir; do + repo_dir="${dir:h}" + rel="${repo_dir#$projects_dir/}" projects+=("$rel") - done + done < <(find "$projects_dir" -maxdepth 3 -name ".git" -type d -not -path "*/.worktrees/*" -print0 2>/dev/null) _describe -t projects 'project' projects && return 0 ;; account/default|acct/default|account/remove|acct/remove|account/rename|acct/rename|account/sync|acct/sync) diff --git a/lib/launcher/menu.zsh b/lib/launcher/menu.zsh index 5d008d2..39768a4 100644 --- a/lib/launcher/menu.zsh +++ b/lib/launcher/menu.zsh @@ -93,12 +93,12 @@ _ckipper_launcher_discover_projects() { local projects_dir="${CKIPPER_PROJECTS_DIR:-$HOME/Developer}" [[ -d "$projects_dir" ]] || return 0 local dir repo_dir rel - for dir in $(find "$projects_dir" -maxdepth "$_CKIPPER_LAUNCHER_PROJECTS_MAXDEPTH" \ - -name ".git" -type d -not -path "*/.worktrees/*" 2>/dev/null); do + while IFS= read -r -d '' dir; do repo_dir="${dir:h}" rel="${repo_dir#$projects_dir/}" print -- "$rel" - done + done < <(find "$projects_dir" -maxdepth "$_CKIPPER_LAUNCHER_PROJECTS_MAXDEPTH" \ + -name ".git" -type d -not -path "*/.worktrees/*" -print0 2>/dev/null) } # Run-Claude flow: enumerate projects under `$CKIPPER_PROJECTS_DIR`, prompt diff --git a/lib/worktree/docker-mode.zsh b/lib/worktree/docker-mode.zsh index 2d9ac39..2ef199b 100644 --- a/lib/worktree/docker-mode.zsh +++ b/lib/worktree/docker-mode.zsh @@ -50,7 +50,7 @@ _ckipper_worktree_run_docker_mode() { _ckipper_worktree_docker_build_base_args _ckipper_worktree_docker_add_optional_args "$claude_creds" "$gh_token" _ckipper_worktree_resolve_ports - [[ "$CKIPPER_WT_FLAG_FIREWALL" = true ]] && CKIPPER_WT_DOCKER_ARGS+=( --cap-add=NET_ADMIN -e ENABLE_FIREWALL=1 ) + [[ "$CKIPPER_WT_FLAG_FIREWALL" = "true" ]] && CKIPPER_WT_DOCKER_ARGS+=( --cap-add=NET_ADMIN -e ENABLE_FIREWALL=1 ) CKIPPER_WT_DOCKER_ARGS+=( ckipper-dev ) _ckipper_worktree_docker_expand_command @@ -252,7 +252,7 @@ _ckipper_worktree_docker_expand_command() { _ckipper_worktree_docker_print_banner() { local mode_label="Docker" [[ ${#CKIPPER_WT_COMMAND[@]} -gt 0 ]] && mode_label+=": ${CKIPPER_WT_COMMAND[1]}" - [[ "$CKIPPER_WT_FLAG_FIREWALL" = true ]] && mode_label+=", firewall" + [[ "$CKIPPER_WT_FLAG_FIREWALL" = "true" ]] && mode_label+=", firewall" echo "Starting $mode_label..." echo " Worktree: $CKIPPER_WT_PATH" echo " Ports: ${CKIPPER_WT_RESOLVED_PORTS[*]}" diff --git a/lib/worktree/run.zsh b/lib/worktree/run.zsh index 5e45376..dff8ee8 100644 --- a/lib/worktree/run.zsh +++ b/lib/worktree/run.zsh @@ -30,14 +30,14 @@ _ckipper_worktree_run() { _ckipper_worktree_resolve_account || return $? _ckipper_worktree_resolve_flags "$CKIPPER_WT_ACTIVE_ACCOUNT" - if [[ "$CKIPPER_WT_FLAG_FIREWALL" = true && "$CKIPPER_WT_FLAG_DOCKER" = false ]]; then + if [[ "$CKIPPER_WT_FLAG_FIREWALL" = "true" && "$CKIPPER_WT_FLAG_DOCKER" = "false" ]]; then echo "Error: --firewall requires --docker" >&2 return 1 fi _ckipper_worktree_create_worktree "$CKIPPER_WT_PROJECT" "$CKIPPER_WT_BRANCH" || return $? - if [[ "$CKIPPER_WT_FLAG_DOCKER" = true ]]; then + if [[ "$CKIPPER_WT_FLAG_DOCKER" = "true" ]]; then _ckipper_worktree_run_docker_mode else _ckipper_worktree_run_normal_mode diff --git a/lib/worktree/worktree.zsh b/lib/worktree/worktree.zsh index 366d1d6..09beac9 100644 --- a/lib/worktree/worktree.zsh +++ b/lib/worktree/worktree.zsh @@ -1,7 +1,7 @@ #!/usr/bin/env zsh # Worktree list, remove, and create operations for `ckipper worktree`. -readonly CKIPPER_WT_FIND_MAX_DEPTH=3 +readonly _CKIPPER_WT_FIND_MAX_DEPTH=3 # Print all worktrees under $CKIPPER_WORKTREES_DIR, grouped by project. # @@ -84,7 +84,7 @@ _ckipper_worktree_remove_worktree() { fi local force_flag="" - [[ "$CKIPPER_WT_FLAG_FORCE" = true ]] && force_flag="--force" + [[ "$CKIPPER_WT_FLAG_FORCE" = "true" ]] && force_flag="--force" (cd "$CKIPPER_PROJECTS_DIR/$project" && git worktree remove $force_flag -- "$wt_path" && git branch -D -- "$worktree" 2>/dev/null) || { echo "Failed to remove worktree. Use --force if it has uncommitted changes." >&2 @@ -328,13 +328,14 @@ _ckipper_worktree_post_create_setup() { || echo "Warning: '$install_cmd' failed. You may need to run it manually." fi - for env_file in $(find "$CKIPPER_PROJECTS_DIR/$project" -maxdepth "$CKIPPER_WT_FIND_MAX_DEPTH" -name ".env*" -not -name "*.example" -not -path "*/node_modules/*" -not -path "*/.git/*"); do - local rel_path="${env_file#$CKIPPER_PROJECTS_DIR/$project/}" - local dest_dir="$CKIPPER_WT_PATH/$(dirname "$rel_path")" + local env_file rel_path dest_dir + while IFS= read -r -d '' env_file; do + rel_path="${env_file#$CKIPPER_PROJECTS_DIR/$project/}" + dest_dir="$CKIPPER_WT_PATH/$(dirname "$rel_path")" mkdir -p "$dest_dir" cp "$env_file" "$dest_dir/" echo "Copied $rel_path" - done + done < <(find "$CKIPPER_PROJECTS_DIR/$project" -maxdepth "$_CKIPPER_WT_FIND_MAX_DEPTH" -name ".env*" -not -name "*.example" -not -path "*/node_modules/*" -not -path "*/.git/*" -print0) _ckipper_worktree_sync_project_registry "$project" } From 01e9c30300337b1c41b62849e2c8500933401b33 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:18:29 -0600 Subject: [PATCH 13/16] fix(config): honor --help short-circuit on every subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract in ckipper.zsh:191-193 ("Run \`ckipper --help\` for per-subcommand details") held for the account and worktree dispatchers but not for config — running \`ckipper config get --help\` returned "Unknown flag: '--help'" because get/set/unset/list/edit forwarded the flag straight to the handler. Mirror the account/worktree pattern: short-circuit ` --help` to `_ckipper_config_help` before dispatching the handler. Five new bats cases — one per subcommand — assert the exit-zero + overview text. Other dispatchers (account, worktree, setup, run) already comply; audited as part of this change. --- lib/config/dispatcher.zsh | 15 ++++++++----- lib/config/dispatcher_test.bats | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/lib/config/dispatcher.zsh b/lib/config/dispatcher.zsh index 8cc486f..415c6b4 100644 --- a/lib/config/dispatcher.zsh +++ b/lib/config/dispatcher.zsh @@ -22,11 +22,16 @@ _ckipper_config_dispatch() { local cmd="$1" shift 2>/dev/null case "$cmd" in - get) _ckipper_config_get "$@" ;; - set) _ckipper_config_set "$@" ;; - unset) _ckipper_config_unset "$@" ;; - list) _ckipper_config_list "$@" ;; - edit) _ckipper_config_edit "$@" ;; + get|set|unset|list|edit) + # Honour the ` --help` contract documented in + # ckipper.zsh — short-circuit before invoking the handler so the + # user sees usage instead of a "missing required arg" error. + if [[ "$1" == "--help" || "$1" == "-h" ]]; then + _ckipper_config_help + return 0 + fi + "_ckipper_config_${cmd}" "$@" + ;; ""|help|-h|--help) _ckipper_config_help ;; *) _ckipper_config_unknown "$cmd"; return 1 ;; esac diff --git a/lib/config/dispatcher_test.bats b/lib/config/dispatcher_test.bats index 01be6fe..1045ea5 100644 --- a/lib/config/dispatcher_test.bats +++ b/lib/config/dispatcher_test.bats @@ -84,3 +84,42 @@ _run_config_dispatch() { [ "$status" -ne 0 ] [[ "$output" =~ "config help" ]] } + +# Regression: per the contract documented in ckipper.zsh:191-193, every +# namespace dispatcher must accept ` --help` as a synonym for +# overview help. Account/worktree dispatchers honoured this; config did not +# — `ckipper config get --help` returned "Unknown flag: '--help'". +@test "dispatcher routes 'set --help' to namespace help (does not run set)" { + _run_config_dispatch "_ckipper_config_dispatch set --help" + + [ "$status" -eq 0 ] + [[ "$output" =~ "ckipper config" ]] +} + +@test "dispatcher routes 'get -h' to namespace help" { + _run_config_dispatch "_ckipper_config_dispatch get -h" + + [ "$status" -eq 0 ] + [[ "$output" =~ "ckipper config" ]] +} + +@test "dispatcher routes 'unset --help' to namespace help" { + _run_config_dispatch "_ckipper_config_dispatch unset --help" + + [ "$status" -eq 0 ] + [[ "$output" =~ "ckipper config" ]] +} + +@test "dispatcher routes 'list --help' to namespace help" { + _run_config_dispatch "_ckipper_config_dispatch list --help" + + [ "$status" -eq 0 ] + [[ "$output" =~ "ckipper config" ]] +} + +@test "dispatcher routes 'edit --help' to namespace help" { + _run_config_dispatch "_ckipper_config_dispatch edit --help" + + [ "$status" -eq 0 ] + [[ "$output" =~ "ckipper config" ]] +} From 9bb2986bc7fc8668d5dc625a835a6a6f73e4f61f Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:36:46 -0600 Subject: [PATCH 14/16] fix(hooks): unify .git path-protection regex between Bash and Edit/Write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bash-guardrails.sh:78` blocked only `.git/info/attributes` and `.git/info/exclude`; `protect-claude-config.sh:47` blocked all of `.git/info/`. The two hooks gated the same threat (a container-side edit landing on a host-side `.git/` file that executes on the next git invocation) but disagreed on the surface — Slice 5 of the develop review flagged this as the load-bearing inconsistency. Align the inner subpath alternation: .git/(config|info/|hooks/|worktrees/) The leading anchor still differs (Bash sees command strings, Edit/Write sees realpath-resolved file paths), but the protected-path list is identical now. Cross-reference comments added to both files so future drift is harder to introduce silently. Three new bats cases: writes anywhere under `.git/info/` are blocked, writes to `.git/info/attributes` are blocked, reads of `.git/info/` files still pass through. --- hooks/bash-guardrails.sh | 9 +++++++-- hooks/bash-guardrails_test.bats | 24 ++++++++++++++++++++++++ hooks/protect-claude-config.sh | 1 + 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/hooks/bash-guardrails.sh b/hooks/bash-guardrails.sh index 0ae4cbd..62a207b 100755 --- a/hooks/bash-guardrails.sh +++ b/hooks/bash-guardrails.sh @@ -74,8 +74,13 @@ if echo "$NORMALIZED" | grep -qE 'git\s+config\s+--(local|worktree)\s'; then fi fi -# 5. .git/hooks, .git/config, and .git/worktrees modification (execute on host) -if echo "$NORMALIZED" | grep -qE '\.git/(hooks|config|info/(attributes|exclude)|worktrees)'; then +# 5. .git/hooks, .git/config, .git/info/, and .git/worktrees modification. +# These execute on the host on the next git invocation. Pattern kept in sync +# with hooks/protect-claude-config.sh:47 — the leading-anchor differs (Bash +# sees command strings, the Edit/Write hook sees realpath-resolved file paths) +# but the inner subpath alternation is the same so both hooks agree on which +# parts of .git/ are protected. +if echo "$NORMALIZED" | grep -qE '\.git/(config|info/|hooks/|worktrees/)'; then if echo "$NORMALIZED" | grep -qE '^(cat|less|head|tail|grep|rg|wc|ls|file|stat|git)\s'; then # Allow reads but block output redirects (cat > .git/hooks/x is a write, not a read) if ! echo "$NORMALIZED" | grep -qE '>'; then diff --git a/hooks/bash-guardrails_test.bats b/hooks/bash-guardrails_test.bats index c7f3659..e56c32e 100644 --- a/hooks/bash-guardrails_test.bats +++ b/hooks/bash-guardrails_test.bats @@ -99,3 +99,27 @@ _run_guardrails() { [ "$status" -eq 2 ] [[ "$output" =~ "force" ]] } + +# Path-protection regex should agree with hooks/protect-claude-config.sh +# on which .git/ subpaths are blocked. Slice 5 of the develop-branch +# review flagged the prior divergence — Bash blocked only +# `info/(attributes|exclude)`, Edit/Write blocked all of `info/`. +@test "bash-guardrails blocks writes anywhere under .git/info/ (matches Edit/Write hook)" { + _run_guardrails "echo bad > .git/info/refs" + + [ "$status" -eq 2 ] + [[ "$output" =~ "Blocked" ]] +} + +@test "bash-guardrails blocks writes to .git/info/attributes" { + _run_guardrails "echo bad-pattern > .git/info/attributes" + + [ "$status" -eq 2 ] + [[ "$output" =~ "Blocked" ]] +} + +@test "bash-guardrails still allows reading .git/info/ files" { + _run_guardrails "cat .git/info/exclude" + + [ "$status" -eq 0 ] +} diff --git a/hooks/protect-claude-config.sh b/hooks/protect-claude-config.sh index 47b06d9..391b665 100755 --- a/hooks/protect-claude-config.sh +++ b/hooks/protect-claude-config.sh @@ -44,6 +44,7 @@ fi # .git/worktrees/ execute on the host the next time the user runs git, so they # constitute a container-escape vector. The leading '/' anchor avoids # over-blocking '.gitignore', '.github/', or directories like '.git-foo/'. +# Pattern subpath kept in sync with hooks/bash-guardrails.sh:78. if [[ $RESOLVED_PATH =~ /\.git/(config|info/|hooks/|worktrees/) ]]; then echo "Blocked: cannot modify $FILE_PATH (protected host .git file)" >&2 exit 2 From c535ca703079b929dd9411fb8590e68d02bb159c Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:36:56 -0600 Subject: [PATCH 15/16] chore(ci): add minimal token permissions and a job timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two standard supply-chain hardening steps for the workflow: 1. Top-level `permissions: contents: read`. The default GITHUB_TOKEN on `push` carries write access to the repo; with no opt-in permissions block any action invoked here would inherit that. The `make lint` / `make test-unit` job only needs to read source — make that explicit so a future regression that pulls in a malicious action can't escalate silently. 2. `timeout-minutes: 15` on the lint-and-test job. A hung step (network stall in `pip install`, an infinite loop in a test) currently runs for 6 hours before GitHub kills it. 15 min is generous: current CI is well under 5 min including brew install + pip install + the full bats + ruff + pytest passes. SHA-pinning `actions/checkout@v4` to a commit hash is the third standard hardening but I'm leaving that as a follow-up so we can pick a known-good SHA from the actions/checkout release page rather than guessing. --- .github/workflows/ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bc452f8..da09e3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,9 +5,15 @@ on: push: branches: [develop, main] +# Default to read-only permissions for the GITHUB_TOKEN. Individual jobs +# that need write access (e.g. release publishing) must opt in explicitly. +permissions: + contents: read + jobs: lint-and-test: runs-on: macos-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v4 From f5a8ac345cb8b912c78ac1538e30d89bf83c9dd9 Mon Sep 17 00:00:00 2001 From: Matt White Date: Sun, 3 May 2026 23:41:12 -0600 Subject: [PATCH 16/16] =?UTF-8?q?fix:=20phase-3=20polish=20=E2=80=94=20par?= =?UTF-8?q?am=20cap,=20stderr=20discipline,=20unguarded=20cd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small follow-ups from the develop-branch review: 1. `_ckipper_account_list_row` took 4 positional args (.claude/rules 3-param cap). Move `default` into a module-level `_CKIPPER_ACCOUNT_LIST_DEFAULT` global that `_ckipper_account_list` populates once per invocation; the row helper drops to 3 args + 1 documented context read. Same pattern already in use elsewhere in this module (_CKIPPER_FINALIZE_CTX, _CKIPPER_RENAME_CTX). 2. `lib/worktree/build-image.zsh:10` and `lib/worktree/ports.zsh:41` wrote error/warning text to stdout. Per the project checklist, error paths surface to monitoring — move both to stderr (`>&2`). 3. `lib/worktree/normal-mode.zsh` `cd` calls were unguarded. If the worktree path went missing between create and run, the command silently executed in the caller's directory (and the trailing restoring `cd "$old_pwd"` likewise). Add an upfront existence check, gate each `cd` with `|| return 1` (or `|| true` for the best-effort restore), and surface a stderr error when the path is missing. Bats remains green (501). --- lib/account/account-management.zsh | 20 +++++++++++++++----- lib/worktree/build-image.zsh | 2 +- lib/worktree/normal-mode.zsh | 10 +++++++--- lib/worktree/ports.zsh | 2 +- 4 files changed, 24 insertions(+), 10 deletions(-) diff --git a/lib/account/account-management.zsh b/lib/account/account-management.zsh index ec02ce7..4d55533 100644 --- a/lib/account/account-management.zsh +++ b/lib/account/account-management.zsh @@ -11,6 +11,12 @@ typeset -gA _CKIPPER_FINALIZE_CTX # Fields: old_dir, new_dir typeset -gA _CKIPPER_RENAME_CTX +# Module-level context for `ckipper account list`: the default account name, +# read once by `_ckipper_account_list` and read by `_ckipper_account_list_row` +# to pick the marker. Lets the row helper stay at 3 positional args (the +# 3-parameter cap from .claude/rules/code-style.md). +typeset -g _CKIPPER_ACCOUNT_LIST_DEFAULT="" + # Validate the account name and --adopt flag from `ckipper account add` arguments. # Prints error messages to stdout and returns non-zero on failure. # @@ -340,14 +346,13 @@ _ckipper_account_list() { return 0 fi _core_registry_check_version || return 1 - local default - default=$(jq -r '.default // ""' "$CKIPPER_REGISTRY") + _CKIPPER_ACCOUNT_LIST_DEFAULT=$(jq -r '.default // ""' "$CKIPPER_REGISTRY") _core_style_header "Registered accounts" _ckipper_account_list_header _core_style_divider jq -r '.accounts | to_entries[] | "\(.key)\t\(.value.config_dir)\t\(.value.keychain_service // "null")"' "$CKIPPER_REGISTRY" | \ while IFS=$'\t' read -r name dir keychain; do - _ckipper_account_list_row "$name" "$dir" "$keychain" "$default" + _ckipper_account_list_row "$name" "$dir" "$keychain" done echo "" echo "* = default. Run: ckipper account default " @@ -371,12 +376,17 @@ _ckipper_account_list_short_dir() { # $1 — account name # $2 — config directory # $3 — keychain service ("null" string when unset) -# $4 — default account name +# +# Reads `_CKIPPER_ACCOUNT_LIST_DEFAULT` (set by `_ckipper_account_list`) to +# decide whether to mark this row as the default. Threading default through +# the registry-stream pipeline as a 4th positional would break the +# 3-parameter cap. # # Returns: # 0 always. _ckipper_account_list_row() { - local name="$1" dir="$2" keychain="$3" default="$4" + local name="$1" dir="$2" keychain="$3" + local default="$_CKIPPER_ACCOUNT_LIST_DEFAULT" local short_dir; short_dir=$(_ckipper_account_list_short_dir "$dir") local email="-" if [[ -f "$dir/.claude.json" ]]; then diff --git a/lib/worktree/build-image.zsh b/lib/worktree/build-image.zsh index d57438a..1943892 100644 --- a/lib/worktree/build-image.zsh +++ b/lib/worktree/build-image.zsh @@ -7,7 +7,7 @@ _ckipper_worktree_build_image() { local docker_dir="${CKIPPER_DIR:-$HOME/.ckipper}/docker" if [[ ! -f "$docker_dir/Dockerfile" ]]; then - echo "Dockerfile not found: $docker_dir/Dockerfile" + echo "Dockerfile not found: $docker_dir/Dockerfile" >&2 return 1 fi echo "Building ckipper-dev Docker image..." diff --git a/lib/worktree/normal-mode.zsh b/lib/worktree/normal-mode.zsh index 5ddb7ee..6c4b25d 100644 --- a/lib/worktree/normal-mode.zsh +++ b/lib/worktree/normal-mode.zsh @@ -6,8 +6,12 @@ # Reads globals: CKIPPER_WT_PATH, CKIPPER_WT_BRANCH, CKIPPER_WT_COMMAND. # Returns: 0 on cd; exit code of command if one was given. _ckipper_worktree_run_normal_mode() { + if [[ ! -d "$CKIPPER_WT_PATH" ]]; then + echo "Error: worktree path missing: $CKIPPER_WT_PATH" >&2 + return 1 + fi if [[ ${#CKIPPER_WT_COMMAND[@]} -eq 0 ]]; then - cd "$CKIPPER_WT_PATH" + cd "$CKIPPER_WT_PATH" || return 1 return 0 fi @@ -16,9 +20,9 @@ _ckipper_worktree_run_normal_mode() { fi local old_pwd="$PWD" - cd "$CKIPPER_WT_PATH" + cd "$CKIPPER_WT_PATH" || return 1 "${CKIPPER_WT_COMMAND[@]}" local exit_code=$? - cd "$old_pwd" + cd "$old_pwd" || true return $exit_code } diff --git a/lib/worktree/ports.zsh b/lib/worktree/ports.zsh index 7e03fa1..3c4b955 100644 --- a/lib/worktree/ports.zsh +++ b/lib/worktree/ports.zsh @@ -38,7 +38,7 @@ _ckipper_worktree_bind_port() { done if [[ "$is_bound" != "true" ]]; then - echo " Port $port: no available host port found ($port-$((port+_CKIPPER_WT_MAX_PORT_FALLBACK_ATTEMPTS-1)) all in use)" + echo " Port $port: no available host port found ($port-$((port+_CKIPPER_WT_MAX_PORT_FALLBACK_ATTEMPTS-1)) all in use)" >&2 fi }