fix(skills): safely repair nested self-links

.github/workflows/ci.yml

@@ -24,13 +24,21 @@ jobs:
 
       - name: Check shell helper syntax
         run: |
+          bash -n scripts/sync-skills
+          bash -n scripts/test-sync-skills
           find skills -path '*/scripts/*' -type f -print0 |
             while IFS= read -r -d '' script; do
               if head -n 1 "$script" | grep -q 'bash'; then
                 bash -n "$script"
               fi
             done
 
+      - name: Test skill link sync and audit
+        run: |
+          sudo apt-get update
+          sudo apt-get install -y ripgrep
+          /bin/bash scripts/test-sync-skills
+
       - name: Test ClawSweeper status helper
         run: skills/clawsweeper-status/scripts/clawsweeper-status.test.sh
 

CHANGELOG.md

@@ -6,6 +6,7 @@ summary: Timeline of guardrail helper changes mirrored from Sweetistics and rela
 
 ## Unreleased
 
+- Fixed skill sync nesting links inside locally owned directories; added bounded allowlisted self-link repair and read-only audit detection.
 - Removed the machine-specific 1Password skill from the public skill set; local discovery now uses its private owner.
 - Fixed `clawsweeper-status` aborting before activity sections on large workflow snapshots while preserving row caps and upstream errors.
 - Fixed `clawsweeper-status` public queue parsing, preserved optional health fields without shifted columns, and added a separate publication-tail summary.

README.md

@@ -30,6 +30,21 @@ Global discovery is built by `scripts/sync-skills` (idempotent; run on every Mac
 - Codex scans nested dirs, so it gets whole-root links: `~/.codex/skills/agent-scripts -> ~/Projects/agent-scripts/skills`, `~/.codex/skills/manager -> ~/Projects/manager/skills`.
 - Claude Code loads only `~/.claude/skills/<name>/SKILL.md` (exactly one level deep; per-entry symlinks are followed, category subfolders are not scanned — verified on 2.1.197). It gets a flat per-skill link mirror covering both repos plus machine-local `~/.codex/skills/<name>` extras.
 - Name collisions resolve agent-scripts > manager > codex-local; the script prints skipped duplicates and prunes broken/stale managed links.
+- Real destination files and directories are preserved. A real Claude skill directory with a Codex backlink to that same directory satisfies local ownership; other real destination conflicts are reported and make sync fail.
+
+For the specific legacy topology `~/.claude/skills/NAME/NAME -> ~/.codex/skills/NAME -> ~/.claude/skills/NAME`, invoke the sync owner directly with an explicit allowlist:
+
+```bash
+/absolute/path/to/agent-scripts/scripts/sync-skills --repair-nested-self-links --dry-run -- boxd-cli boxd-setup-deploy
+```
+
+```bash
+/absolute/path/to/agent-scripts/scripts/sync-skills --repair-nested-self-links -- boxd-cli boxd-setup-deploy
+```
+
+This mode validates every candidate before unlinking only the extra nested leaves. It preserves the real skill directories, assets, and valid Codex backlinks, and exits before creating roots, building mirrors, pruning, or touching instruction pointers. Names must start with an ASCII letter or digit and contain only letters, digits, `.`, `_`, or `-`; duplicates, missing names, and unknown arguments are rejected. A missing nested leaf is a no-op only with the expected surrounding topology. Redirected/inaccessible roots, unexpected objects or literal targets, and changed directory/link identities cause refusal. Rechecks before each unlink are not atomic concurrency protection; a later error stops the batch and reports removals already completed, without rollback.
+
+The read-only `skills/fleet-maintenance/scripts/agent-skill-links-audit.sh` reports same-name nested ancestor loops as `reason=nested-self-link`. This is narrow detection, not an exhaustive graph validator. Its `--repair` remains a broad sync through `~/Projects/agent-scripts/scripts/sync-skills`; it is not the scoped repair above. Run `scripts/test-sync-skills` for isolated fixture coverage; `scripts/test-sync-skills --recurrence-only /absolute/path/to/old-sync-skills` runs the unchanged recurrence assertion against an original helper.
 
 Shared personal skills live as real folders in `skills/`. Public OpenClaw shared skills live in `../agent-skills` and are exposed here with tracked relative symlinks. Repo-owned skills stay canonical in their repo and are exposed here the same way, for example:
 
@@ -62,6 +77,7 @@ Repo-specific rules go below that pointer. Do not copy the shared blocks into do
 `scripts/sync-skills`
 - Builds the per-machine skill mirror: Codex whole-root links, Claude flat per-skill links, shared `AGENTS.MD` pointers.
 - Idempotent; prints changes only, prunes broken/stale managed links, never clobbers real files.
+- No arguments runs ordinary sync; `--help` prints usage. Only the scoped `--repair-nested-self-links` mode accepts `--dry-run`.
 
 `scripts/validate-skills`
 - Checks every `skills/*/SKILL.md`.

scripts/sync-skills

@@ -8,17 +8,140 @@
 # Collision priority: agent-scripts > manager > codex-local extras.
 set -eo pipefail
 
+usage() {
+  printf 'usage: %s [--help | --repair-nested-self-links [--dry-run] -- NAME...]\n' "$0"
+}
+
+repair=0
+dry_run=0
+if [ "$#" -gt 0 ]; then
+  case "$1" in
+    --help) [ "$#" -eq 1 ] || { usage >&2; exit 2; }; usage; exit 0 ;;
+    --repair-nested-self-links)
+      repair=1
+      shift
+      if [ "${1:-}" = --dry-run ]; then dry_run=1; shift; fi
+      [ "${1:-}" = -- ] || { usage >&2; exit 2; }
+      shift
+      [ "$#" -gt 0 ] || { usage >&2; exit 2; }
+      repair_names=()
+      for name in "$@"; do
+        case "$name" in
+          ''|[!a-zA-Z0-9]*|*[!a-zA-Z0-9._-]*)
+            printf 'invalid skill basename: %s\n' "$name" >&2; exit 2 ;;
+        esac
+        for previous in "${repair_names[@]}"; do
+          [ "$name" != "$previous" ] || { printf 'duplicate skill name: %s\n' "$name" >&2; exit 2; }
+        done
+        repair_names+=("$name")
+      done
+      ;;
+    *) usage >&2; exit 2 ;;
+  esac
+fi
+
 AGENT_SKILLS="$HOME/Projects/agent-scripts/skills"
 MANAGER_SKILLS="$HOME/Projects/manager/skills"
 CODEX_ROOT="$HOME/.codex/skills"
 CLAUDE_ROOT="$HOME/.claude/skills"
 AGENTS_MD="$HOME/Projects/agent-scripts/AGENTS.MD"
 
+# This mode exits before any ordinary mirror or instruction-pointer work.
+if [ "$repair" -eq 1 ]; then
+  removed=0
+  absent=0
+  refuse() {
+    printf 'nested-self-links: refused: %s (removed=%s already-absent=%s)\n' "$*" "$removed" "$absent" >&2
+    exit 1
+  }
+  # BSD and GNU stat both default to the link itself, not its target.
+  if stat -c '%d:%i' "$HOME" >/dev/null 2>&1; then
+    stat_args=(-c '%d:%i')
+  else
+    stat_args=(-f '%d:%i')
+  fi
+  identity() { stat "${stat_args[@]}" "$1"; }
+  roots=("$HOME" "$HOME/.claude" "$HOME/.codex" "$CLAUDE_ROOT" "$CODEX_ROOT")
+  root_ids=()
+  check_roots() {
+    local i root physical current
+    for i in "${!roots[@]}"; do
+      root=${roots[$i]}
+      [ ! -L "$root" ] && [ -d "$root" ] && [ -r "$root" ] && [ -x "$root" ] || refuse "not an accessible real root: $root"
+      physical=$(cd "$root" && pwd -P) || refuse "cannot resolve root: $root"
+      [ "$physical" = "$root" ] || refuse "redirected root: $root"
+      current=$(identity "$root") || refuse "cannot identify root: $root"
+      [ "$current" = "${root_ids[$i]:-$current}" ] || refuse "root changed: $root"
+      root_ids[$i]=$current
+    done
+  }
+  check_candidate() {
+    local parent="$CLAUDE_ROOT/$1" backlink="$CODEX_ROOT/$1" nested="$CLAUDE_ROOT/$1/$1" target
+    [ ! -L "$parent" ] && [ -d "$parent" ] && [ -r "$parent" ] && [ -x "$parent" ] || refuse "not an accessible real skill directory: $parent"
+    [ -f "$parent/SKILL.md" ] && [ -r "$parent/SKILL.md" ] || refuse "missing or unreadable SKILL.md: $parent"
+    [ -L "$backlink" ] || refuse "not a Codex backlink: $backlink"
+    target=$(readlink "$backlink") || refuse "cannot read backlink: $backlink"
+    [ "$target" = "$parent" ] && [ "$parent" -ef "$backlink" ] || refuse "unexpected Codex backlink: $backlink"
+    candidate_state=absent
+    candidate_leaf_id=absent
+    if [ -L "$nested" ]; then
+      target=$(readlink "$nested") || refuse "cannot read nested link: $nested"
+      [ "$target" = "$backlink" ] && [ "$parent" -ef "$nested" ] && [ "$backlink" -ef "$nested" ] || refuse "unexpected nested link: $nested"
+      candidate_state=present
+      candidate_leaf_id=$(identity "$nested") || refuse "cannot identify nested link: $nested"
+    elif [ -e "$nested" ]; then
+      refuse "nested entry is not a symlink: $nested"
+    fi
+    candidate_parent_id=$(identity "$parent") || refuse "cannot identify skill directory: $parent"
+    candidate_backlink_id=$(identity "$backlink") || refuse "cannot identify backlink: $backlink"
+  }
+  check_roots
+  parent_ids=(); backlink_ids=(); leaf_ids=(); states=()
+  for i in "${!repair_names[@]}"; do
+    check_candidate "${repair_names[$i]}"
+    parent_ids[$i]=$candidate_parent_id
+    backlink_ids[$i]=$candidate_backlink_id
+    leaf_ids[$i]=$candidate_leaf_id
+    states[$i]=$candidate_state
+  done
+  for i in "${!repair_names[@]}"; do
+    name=${repair_names[$i]}
+    nested="$CLAUDE_ROOT/$name/$name"
+    # Recheck the roots and this candidate immediately before unlinking. These
+    # shell checks detect drift but cannot make pathname-based unlink atomic.
+    check_roots
+    check_candidate "$name"
+    [ "$candidate_parent_id" = "${parent_ids[$i]}" ] &&
+      [ "$candidate_backlink_id" = "${backlink_ids[$i]}" ] &&
+      [ "$candidate_leaf_id" = "${leaf_ids[$i]}" ] &&
+      [ "$candidate_state" = "${states[$i]}" ] || refuse "topology changed: $name"
+    if [ "$candidate_state" = absent ]; then
+      absent=$((absent + 1))
+      printf 'already-absent %s\n' "$nested"
+    elif [ "$dry_run" -eq 1 ]; then
+      printf 'would-remove %s\n' "$nested"
+    else
+      unlink "$nested" || refuse "unlink failed: $nested"
+      removed=$((removed + 1))
+      printf 'removed %s\n' "$nested"
+    fi
+  done
+  [ "$dry_run" -eq 1 ] || printf 'nested-self-links: removed=%s already-absent=%s\n' "$removed" "$absent"
+  exit 0
+fi
+
 changed=0
+conflicts=0
 note() { printf '%s\n' "$*"; changed=1; }
 
 # link <target> <linkpath>: create/retarget symlink, quiet when already right.
 link() {
+  if [ ! -L "$2" ] && [ -e "$2" ]; then
+    [ -d "$1" ] && [ -d "$2" ] && [ "$1" -ef "$2" ] && return 0
+    printf 'WARN: real destination preserved: %s (expected -> %s)\n' "$2" "$1" >&2
+    conflicts=$((conflicts + 1))
+    return 0
+  fi
   [ "$(readlink "$2" 2>/dev/null)" = "$1" ] && return 0
   ln -sfn "$1" "$2"
   note "link $2 -> $1"
@@ -87,17 +210,11 @@ for entry in "$CLAUDE_ROOT"/* "$CLAUDE_ROOT"/.[!.]*; do
   esac
 done
 
-# --- Global instruction pointers; never clobber a real file.
-ensure_md() {
-  if [ -e "$1" ] && [ ! -L "$1" ]; then
-    printf 'WARN: %s is a real file, not linking (expected -> %s)\n' "$1" "$AGENTS_MD"
-    return 0
-  fi
-  link "$AGENTS_MD" "$1"
-}
-ensure_md "$HOME/.claude/CLAUDE.md"
-ensure_md "$HOME/.claude/AGENTS.md"
-ensure_md "$HOME/.codex/AGENTS.md"
+# --- Global instruction pointers use the same real-destination protection.
+link "$AGENTS_MD" "$HOME/.claude/CLAUDE.md"
+link "$AGENTS_MD" "$HOME/.claude/AGENTS.md"
+link "$AGENTS_MD" "$HOME/.codex/AGENTS.md"
 
+[ "$conflicts" -eq 0 ] || { printf 'skills mirror has %s real destination conflict(s)\n' "$conflicts" >&2; exit 1; }
 [ "$changed" = 0 ] && echo "skills mirror up to date (${#names[@]} skills)"
 exit 0