fix(agents-vault): keep the knowledge walk and the launch push inside their bounds

The agy knowledge allowlist walked the store with `**` and copied with plain cp, so a symlink inside the store was both followed and dereferenced. The extension rule still bounded what kind of file was collected, but not whose: a link to a home directory hands over settings.json, CLAUDE.md and every cached .json in it, and those reached a commit. A link to / made the walk itself unbounded, on the path that runs before every agent launch. The tree is now walked a level at a time and nothing that is a symlink is followed or copied. Autopush had the same shape one layer out. Neither GIT_TERMINAL_PROMPT nor GIT_ASKPASS closes a socket, and git has no connect timeout to set: against a blackholed address a push took 135s with http.lowSpeedLimit and http.lowSpeedTime set as well as without them. ssh can time itself out and is now told to; the autopush pull and push are additionally capped with timeout(1). An explicit --push stays uncapped, since it is watched and has to report what a real transfer really did. Also: scaffold /.migrate-stash into .gitignore beside /.adopt-stash, which the comment already claimed was covered; and drop the live memory path during a slug migration only when it is a link. Reached from the path-derived fallback candidate it can be a real populated directory, where rm -f correctly refuses -- but said so in rm's voice, so a --silent run that had succeeded printed what read as an error.
2026-09-03 18:56:46 -04:00
parent 16ea31289d
commit d9b56790c5
3 changed files with 217 additions and 14 deletions
@@ -170,7 +170,9 @@ full sub-category breakdown of every category.

    When set to 1, agents-vault also pushes on wrapper launch. Defaults to
    off: the vault commits locally on every launch and pushes from the
-    Claude Code SessionEnd hook or an explicit agents-vault --push.
+    Claude Code SessionEnd hook or an explicit agents-vault --push. That
+    push is synchronous, so with autopush on the pull and the push are
+    each capped at 20 seconds; an explicit --push is left uncapped.

 NOTE:
 With autopush off and no SessionEnd hook installed, backups accumulate
@@ -38,7 +38,9 @@
 #   allowlist runs all the way down, not just at the top: inside agy's
 #   knowledge store only *.md and *.json files are copied, so a credential
 #   file or a conversation database appearing there is left behind by the
-#   same rule rather than by being known about in advance.
+#   same rule rather than by being known about in advance. Symlinks found
+#   inside the store are neither followed nor copied, so the allowlist
+#   bounds whose files it collects and not merely what kind.
 #
 #   Global state that belongs to no project is tracked as well. Claude's
 #   global memory directory (~/.claude/memory) is symlinked into the vault
@@ -131,9 +133,13 @@
 #
 # NOTES
 #   Set __fish_agent_vault_dir to relocate the vault. Set
-#   __fish_agent_vault_autopush to 1 to also push on wrapper launch;
-#   it defaults to off so a backgrounded push can never hang or prompt
-#   invisibly underneath a starting agent.
+#   __fish_agent_vault_autopush to 1 to also push on wrapper launch; it
+#   defaults to off because that push is synchronous and so delays every
+#   launch. With it on, the pull and the push are each capped at 20
+#   seconds, since git has no connect timeout of its own and an
+#   unreachable remote otherwise blocks for minutes. An explicit --push
+#   is left uncapped: it is watched, and it must report what a real
+#   transfer really did.
 #
 #   --adopt rebinds an entry; it does not pin its name. The slug is
 #   re-derived from the project on every run, so the next ordinary run
@@ -329,10 +335,12 @@ function agents-vault --description 'track curated agent memory in a host-scoped
            '*.db-wal' \
            '*.db-shm' \
            '' \
-            '# --adopt stashes the entry it is about to overwrite inside .git/,' \
-            '# out of reach of `git add -A`. This covers the fallback location' \
-            '# it uses when .git is not a directory.' \
-            '/.adopt-stash' >"$vault/.gitignore"
+            '# --adopt and the slug migration each stash the entry they are' \
+            '# about to overwrite inside .git/, out of reach of `git add -A`.' \
+            '# These cover the fallback locations they use when .git is not a' \
+            '# directory.' \
+            '/.adopt-stash' \
+            '/.migrate-stash' >"$vault/.gitignore"
        set changed 1
    end

@@ -627,7 +635,39 @@ function agents-vault --description 'track curated agent memory in a host-scoped
        # Fish wildcards skip dot-led names at every path component, so
        # dotfiles and hidden subdirectories are already out; the extension
        # allowlist is what keeps them out on purpose rather than by luck.
-        set -l kfiles $knowledge/**.md $knowledge/**.json
+        #
+        # The tree is walked a level at a time rather than globbed with **,
+        # because a symlink has to stop the walk and ** has no way to say
+        # so. A recursive ** descends through a symlinked directory and cp
+        # follows a symlinked file, which between them undo the whole point
+        # of the allowlist twice over. A `ln -s ~ knowledge/x` puts every
+        # qualifying .md and .json in the home directory -- settings.json,
+        # CLAUDE.md, cache and status files -- into the vault and into a
+        # commit: the extension rule still bounds what *kind* of file goes
+        # in, but the store boundary that decides *whose* files they are is
+        # gone. Worse, `ln -s / knowledge/x` makes the walk itself
+        # unbounded, and this runs synchronously in front of every agent
+        # launch. Fish does stop a true self-referential cycle; a symlink
+        # to a merely enormous tree is not a cycle.
+        #
+        # So nothing that is a symlink is ever followed or copied, whether
+        # it names a file or a directory. The knowledge store's own root
+        # may still be a link -- that one is the configured location of the
+        # store rather than something found inside it.
+        set -l kfiles
+        set -l kdirs "$knowledge"
+        while set -q kdirs[1]
+            set -l dir $kdirs[1]
+            set -e kdirs[1]
+            for e in $dir/*
+                test -L "$e"; and continue
+                if test -d "$e"
+                    set -a kdirs "$e"
+                else if string match -qr '\.(md|json)$' -- "$e"
+                    set -a kfiles "$e"
+                end
+            end
+        end
        set -l kfailed 0
        for f in $kfiles
            test -f "$f"; or continue
@@ -836,7 +876,18 @@ function agents-vault --description 'track curated agent memory in a host-scoped
            end
            printf 'renamed: %s → %s (%s)\n' "$prev_slug" "$slug" (date -I) \
                >>"$entry/origin"
-            rm -f "$live"
+            # Only a link is dropped here, and only so the relink below has
+            # somewhere to put the new one. Usually $live is exactly that: a
+            # symlink at the old entry, now dangling. But the migration is
+            # also reachable from the path-derived fallback candidate, and
+            # there $live can be a real, populated directory -- someone's
+            # actual memory. rm -f cannot delete it, which is the right
+            # outcome, but it says so on stderr in rm's own voice, so a
+            # --silent run that succeeded printed what reads as an error.
+            # The directory case needs no removal anyway:
+            # _agents_repo_ensure_symlink copies a populated live directory
+            # into the vault without clobbering before it replaces it.
+            test -L "$live"; and rm -f "$live"
            set changed 1
            test $verbose -eq 1; and echo "$c_ok→ Migrated vault entry $prev_slug → $slug$c_reset"
        end
@@ -930,10 +981,41 @@ function agents-vault --description 'track curated agent memory in a host-scoped
            # helper, which git consults before it ever falls back to
            # prompting; they only close off the interactive last resort,
            # which under a starting agent is indistinguishable from a hang.
+            #
+            # They close the prompt, not the socket, and git has no knob
+            # that closes the socket either: there is no HTTP connect
+            # timeout in its configuration at all, and http.lowSpeedLimit /
+            # http.lowSpeedTime -- the usual suggestion -- only start
+            # counting once bytes are moving, so they expire never against
+            # an address that simply blackholes the SYN. Measured against
+            # 192.0.2.1 with both set: no return inside 30s; unset: 135s.
+            #
+            # ssh is the one transport that can time itself out, so it is
+            # told to, unless the user has already said how to run ssh.
+            # Everything else is bounded from outside with timeout(1).
+            set -l gitenv GIT_TERMINAL_PROMPT=0 GIT_ASKPASS=true
+            set -q GIT_SSH_COMMAND
+            or set -a gitenv 'GIT_SSH_COMMAND=ssh -o ConnectTimeout=10'
+            set -l gitnet env $gitenv
+
+            # Only autopush is wrapped. An explicit --push is a thing the
+            # user asked for and is watching, and it must report the real
+            # exit status of a real transfer, so it is allowed to take as
+            # long as the transfer honestly takes. Autopush runs
+            # synchronously in front of every agent launch, which is the
+            # block this whole design exists to remove; there an
+            # unreachable remote costs a bounded 20s per operation instead
+            # of an open-ended wait. timeout's own 124 is a non-zero exit
+            # like any other, so a bounded push still reports as failed and
+            # the commit it could not send has already landed locally. A
+            # push too slow for the bound can always be run by hand.
+            if not set -q _flag_push; and type -q timeout
+                set gitnet timeout 20 $gitnet
+            end
            set -l reached 1
            if git -C "$vault" rev-parse --abbrev-ref --symbolic-full-name '@{u}' >/dev/null 2>&1
-                if not GIT_TERMINAL_PROMPT=0 GIT_ASKPASS=true \
-                        git -C "$vault" pull --rebase --autostash -q >/dev/null 2>/dev/null
+                if not $gitnet git -C "$vault" pull --rebase --autostash -q \
+                        >/dev/null 2>/dev/null
                    # Two unrelated failures land here and reporting them as
                    # one sends the user hunting for a conflict that never
                    # existed. A rebase that genuinely started and stopped
@@ -956,7 +1038,7 @@ function agents-vault --description 'track curated agent memory in a host-scoped
            # push would only fail a second time, more confusingly, and the
            # pull has already said exactly what went wrong.
            if test $reached -eq 1
-                if GIT_TERMINAL_PROMPT=0 GIT_ASKPASS=true git -C "$vault" push -q origin HEAD
+                if $gitnet git -C "$vault" push -q origin HEAD
                    test $verbose -eq 1; and echo "$c_ok→ Pushed the vault to origin$c_reset"
                else
                    # The commit above did happen, so the memory is safe
@@ -563,6 +563,54 @@ set -l dirty_new_slug git.rootiest.dev-rootiest-dirty
 check "fallback finds sanitized local entry" dirty-precious (cat $vroot2/agent-vault/projects/$dirty_new_slug/claude/memory/keep.md)
 check "fallback old entry removed" false (test -d $vroot2/agent-vault/projects/$dirty_local_slug; and echo true; or echo false)

+# Same fallback path, but the live memory directory is a real populated
+# directory rather than a symlink -- the shape a machine ends up in when
+# an agent wrote memory while the link was missing. The migration then
+# tries to clear the live path out of the relink's way, and clearing a
+# directory is not something it may do: that is somebody's memory, and
+# _agents_repo_ensure_symlink already folds it into the vault without
+# clobbering. Refusing is therefore correct, but refusing in rm's voice
+# is not: a --silent run that did the right thing and returned 0 still
+# printed "rm: cannot remove ...: Is a directory", which is the only
+# thing the user sees and reads as a failure.
+set -l real_root (mktemp -d); set -ga TMPDIRS $real_root
+set -l rp "$real_root/proj"
+mkdir -p "$rp"
+git -C "$rp" init -q
+git -C "$rp" config user.email t@t
+git -C "$rp" config user.name t
+git -C "$rp" config commit.gpgsign false
+git -C "$rp" config core.hooksPath /dev/null
+
+set -l rmangled (string replace -a '/' '-' -- $rp | string replace -a '.' '-')
+mkdir -p $croot2/$rmangled/memory
+echo "banked" >$croot2/$rmangled/memory/old.md
+
+pushd $rp >/dev/null
+agents-vault --silent
+set -l real_local_slug (_agents_repo_slug $rp)
+popd >/dev/null
+
+# Replace the link with a real directory holding memory the vault has
+# never seen, then change the slug so the migration runs.
+rm -f $croot2/$rmangled/memory
+mkdir -p $croot2/$rmangled/memory
+echo "written-live" >$croot2/$rmangled/memory/fresh.md
+git -C $rp remote add origin https://git.rootiest.dev/rootiest/realdir.git
+
+set -l rerr (mktemp); set -ga TMPDIRS $rerr
+pushd $rp >/dev/null
+set -l real_rc (agents-vault --silent 2>$rerr; echo $status)
+popd >/dev/null
+set -l real_err (cat $rerr)
+set -l real_new_slug git.rootiest.dev-rootiest-realdir
+check "real-directory migration succeeds" 0 "$real_rc"
+check "real-directory migration stays silent" "" "$real_err"
+check "real-directory migration keeps banked memory" banked (cat $vroot2/agent-vault/projects/$real_new_slug/claude/memory/old.md 2>/dev/null)
+check "real-directory migration keeps live memory" written-live (cat $vroot2/agent-vault/projects/$real_new_slug/claude/memory/fresh.md 2>/dev/null)
+check "real-directory migration relinks" true (test -L $croot2/$rmangled/memory; and echo true; or echo false)
+check "real-directory old entry removed" false (test -d $vroot2/agent-vault/projects/$real_local_slug; and echo true; or echo false)
+
 set -e __fish_agent_vault_dir
 set -e __fish_agent_vault_claude_root

@@ -699,6 +747,30 @@ printf 'transcript\n' >$agy5/knowledge/session.jsonl
 : >$agy5/knowledge/conversations.db-wal
 : >$agy5/knowledge/conversations.db-shm

+# Symlinks inside the store. The extension allowlist bounds what *kind* of
+# file is collected; it says nothing about whose. A store-relative walk
+# that dereferenced links would pull qualifying .md and .json out of
+# whatever the link names -- a real home directory is full of them -- so
+# the store boundary has to hold on its own. $outside5 stands in for that
+# home: a directory the store has no business reaching into, planted with
+# exactly the shapes that qualify.
+set -l outside5 (mktemp -d); set -ga TMPDIRS $outside5
+mkdir -p $outside5/nested
+echo SECRET-OUTSIDE-KNOWLEDGE >$outside5/leaked.json
+echo SECRET-OUTSIDE-KNOWLEDGE >$outside5/target.md
+echo SECRET-OUTSIDE-KNOWLEDGE >$outside5/nested/deep.md
+ln -s $outside5 $agy5/knowledge/linked
+ln -s $outside5/target.md $agy5/knowledge/alias.md
+# Not a cycle, so a recursive glob's cycle guard does not catch it: a link
+# to the filesystem root simply makes the walk enormous. This one is here
+# for the clock as much as for the contents -- the copy runs synchronously
+# in front of every agent launch, and a `**` glob over this fixture did
+# not return within 20s.
+ln -s / $agy5/knowledge/root
+# A genuine cycle too, since the walk must not depend on the glob's guard.
+mkdir -p $agy5/knowledge/cyc
+ln -s $agy5/knowledge $agy5/knowledge/cyc/loop
+
 # A global (non-per-project) Claude memory directory with a sentinel file.
 # __fish_agent_vault_claude_home is what keeps this off the real ~/.claude:
 # if agents-vault ignored the override, these checks would fail here *and*
@@ -707,9 +779,11 @@ mkdir -p $chome5/memory
 echo global-memory >$chome5/memory/g.md

 set -l gp (new_repo https://git.rootiest.dev/rootiest/globals.git)
+set -l t5_start (date +%s)
 pushd $gp >/dev/null
 agents-vault --silent
 popd >/dev/null
+set -l t5_elapsed (math (date +%s) - $t5_start)

 check "agy knowledge copied" learned (cat $vroot5/agent-vault/global/agy/knowledge/fact.md)
 check "agy settings copied" '{"model":"x"}' (cat $vroot5/agent-vault/global/agy/settings.json)
@@ -726,9 +800,27 @@ end
 # Not merely absent from the worktree: absent from the history, which is
 # what actually leaves the machine on a push.
 check "knowledge: no secret reached a commit" false (git -C $vroot5/agent-vault grep -q SECRET-INSIDE-KNOWLEDGE HEAD -- global 2>/dev/null; and echo true; or echo false)
+
+# Symlinks: nothing the links name may appear, under any name. The linked
+# directory must not exist in the vault at all (following it would recreate
+# its tree wholesale), and the aliased file must not exist either, even
+# though its own name qualifies -- a dereferencing copy writes a real file
+# at the link's name and the extension rule waves it through.
+for escapee in linked linked/leaked.json linked/nested/deep.md alias.md root cyc/loop
+    check "knowledge: symlinked $escapee stayed out" false (test -e $vroot5/agent-vault/global/agy/knowledge/$escapee; and echo true; or echo false)
+end
+check "knowledge: nothing outside the store reached a commit" false (git -C $vroot5/agent-vault grep -q SECRET-OUTSIDE-KNOWLEDGE HEAD 2>/dev/null; and echo true; or echo false)
+# The clock, not the contents: a walk that descends a link to / does not
+# finish, and this is the every-launch path. Generous enough that a loaded
+# machine cannot fail it by being slow.
+check "knowledge: a link to / does not stall the launch path" true (test $t5_elapsed -lt 20; and echo true; or echo false)
 # A torn database with its completing write-ahead log deliberately excluded
 # is worse than no database at all, so the scaffold ignores all three.
 check "scaffolded .gitignore excludes *.db" true (grep -qxF '*.db' $vroot5/agent-vault/.gitignore; and echo true; or echo false)
+# Both stash fallbacks, or a crash mid-rename leaves a shadow copy of an
+# entry sitting at the vault root for the next `git add -A` to commit.
+check "scaffolded .gitignore excludes /.adopt-stash" true (grep -qxF '/.adopt-stash' $vroot5/agent-vault/.gitignore; and echo true; or echo false)
+check "scaffolded .gitignore excludes /.migrate-stash" true (grep -qxF '/.migrate-stash' $vroot5/agent-vault/.gitignore; and echo true; or echo false)

 check "global claude memory in the vault" global-memory (cat $vroot5/agent-vault/global/claude/memory/g.md)
 check "global claude memory is now a link" true (test -L $chome5/memory; and echo true; or echo false)
@@ -1369,6 +1461,33 @@ popd >/dev/null
 set -e __fish_agent_vault_autopush
 check "failing autopush returns non-zero" 1 "$fp2rc"

+# Autopush runs synchronously in front of every agent launch, so it must be
+# bounded. Nothing in git bounds it: there is no HTTP connect timeout in
+# its configuration, http.lowSpeedLimit/http.lowSpeedTime only start
+# counting once bytes move, and GIT_TERMINAL_PROMPT/GIT_ASKPASS close the
+# credential prompt rather than the socket. Unwrapped, this remote took
+# 135s to give up. 192.0.2.1 is TEST-NET-1: reserved, unrouted, and
+# therefore a blackhole rather than a fast refusal. A network that does
+# refuse it quickly makes this pass without proving much, which is the
+# right way round for a test that must never fail spuriously.
+#
+# This test costs its own bound in wall time. That is the price of
+# measuring a timeout, and this defect -- a network call on the launch
+# path -- has now been introduced twice.
+set -l blackhole https://192.0.2.1/vault.git
+agents-vault --remote=$blackhole --silent
+echo pushed-into-the-void >$croot11/$pmang/memory/p6.md
+set -g __fish_agent_vault_autopush 1
+set -l bh_start (date +%s)
+pushd $pp >/dev/null
+set -l bhrc (agents-vault --silent 2>/dev/null; echo $status)
+popd >/dev/null
+set -l bh_elapsed (math (date +%s) - $bh_start)
+set -e __fish_agent_vault_autopush
+check "autopush to a blackholed remote returns non-zero" 1 "$bhrc"
+check "autopush to a blackholed remote is bounded" true (test $bh_elapsed -lt 60; and echo true; or echo false)
+check "a bounded autopush still committed locally" true (git -C $vroot11/agent-vault ls-files --error-unmatch projects/$pslug/claude/memory/p6.md >/dev/null 2>&1; and echo true; or echo false)
+
 set -e __fish_agent_vault_dir
 set -e __fish_agent_vault_claude_root
 set -g __fish_agent_vault_claude_home $HERMETIC_HOME/claude