Develop-branch code-review fixes: 9 Critical bugs + cross-cutting cleanup

.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
 

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
 

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
 

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"
@@ -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)

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
@@ -67,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

hooks/bash-guardrails_test.bats

@@ -42,3 +42,84 @@ _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" ]]
+}
+
+# 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 ]
+}

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

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.
 #
@@ -52,7 +58,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 +72,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: <service>" — 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 +100,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.
@@ -336,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 <name>"
@@ -367,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
@@ -406,19 +420,25 @@ _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'."
 }
 
 # 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"
@@ -427,9 +447,13 @@ _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"
+    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