Stop a failed up from tearing down a working tunnel

The previous commit moved `down` to the top of `up` to fix a resolv.conf
idempotency bug, and in doing so put it *before* every network fetch that can
fail. Re-review caught it and I reproduced it: with a tunnel up, an `up` that
fails on bad credentials left `pia0` gone and traffic silently back on the real
address, while the error talked only about credentials. A privacy regression
introduced by a correctness fix.

`down` now runs after the token, server list and key registration have all
succeeded — nothing above that line touches the network stack — and still
clears the stale backup it was added for.

Everything after it is covered by a rollback. Note this is an EXIT trap with a
flag, not `trap ... ERR`: my first attempt used ERR and did not fire at all,
because ERR is not inherited by shell functions without `set -E`, so a failure
inside add_route missed it, and `die` exits explicitly, which is not an error.
Verified by forcing a route collision — the tunnel is torn down and DNS is
intact, where before the fix it was left half-configured with DNS dead.

Also from the review:

- **The private key lived on disk for the whole life of the tunnel.**
  `commit_container_snapshot` blanks env vars, never files, and nothing tears
  the tunnel down before a recreate or migrate — so the `down`-time cleanup
  never covered the path that put a key in a snapshot in the first place. It is
  now deleted the moment `wg set` has read it; the kernel keeps its own copy,
  verified by checking the interface still works afterwards.
- **`up` claimed success without a handshake.** An unreachable peer still
  routes — into a black hole — so `up --full` could exit 0 having pointed all
  traffic and resolv.conf at a peer that never answered, with `status` printing
  "mode: full tunnel". Now polls for a handshake and rolls back if none arrives.
- **`status` needed root and did not check.** `wg show` fails unprivileged and
  was swallowed, so an unprivileged run printed "no tunnel up" and then "mode:
  full tunnel" in the same breath. An agent reading the first line would re-run
  `up` — which, before the fix above, destroyed the tunnel it failed to see.
- **The killswitch bullet was false.** It said `iptables` is not in the image;
  it is, so a killswitch is buildable. It stays unbuilt because it would cut
  Claude Code's own API traffic — an honest reason, unlike the previous one.
- `install_feature_skill` rejects path-traversal names, not just blank ones —
  verified `../skills` would have deleted the whole skills directory including
  Mission Control's — and reports `mkdir`/`cp` failures instead of printing a
  success line regardless.
- The usage text ended by printing `set -euo pipefail`, off by one line.
- HOW-TO-USE claimed "the container says so on start". entrypoint prints to
  PID 1's stdout, which no terminal or UI surfaces — `docker logs` appears
  nowhere in the repo. Now points at the migration pre-flight, which does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

container/entrypoint.sh

@@ -359,21 +359,31 @@ install_feature_skill() {
     local _src="/opt/triple-c-skills/$1"
     local _dest="/home/claude/.claude/skills/$1"
 
-    # A blank name would make the disabled branch `rm -rf` the whole skills
+    # Reject anything that is not a plain directory name. The disabled branch
-    # directory, Mission Control's included, under a persisted volume.
+    # `rm -rf`s $_dest under a *persisted volume*, so a blank name would take the
-    [ -n "$_name" ] || { echo "entrypoint: install_feature_skill called with no name"; return 1; }
+    # whole skills directory (Mission Control's included) and `../x` would escape
+    # it entirely. Only the literal `pia-vpn` is passed today; this is so that
+    # stays true.
+    case "$_name" in
+        ''|*/*|.*) echo "entrypoint: install_feature_skill: bad skill name '$_name'"; return 1 ;;
+    esac
 
     if [ "$_enabled" = "1" ]; then
         if [ ! -d "$_src" ]; then
             echo "entrypoint: $_name skill unavailable — this container's base image predates it; migrate the project to get it"
             return 0
         fi
-        mkdir -p /home/claude/.claude/skills
+        # Checked, not assumed: with no `set -e` in this script every step here
+        # can fail (full volume, read-only mount, a file where the directory
+        # should be) and the success line would still print.
+        mkdir -p /home/claude/.claude/skills || {
+            echo "entrypoint: $_name skill install FAILED (cannot create ~/.claude/skills)"; return 1; }
         # Not just $_dest: when Mission Control is off nothing else creates the
         # parent, so root would own it and `claude` could not add a skill there.
         chown claude:claude /home/claude/.claude/skills
         rm -rf "$_dest"
-        cp -r "$_src" "$_dest"
+        cp -r "$_src" "$_dest" || {
+            echo "entrypoint: $_name skill install FAILED (copy from $_src)"; return 1; }
         chown -R claude:claude "$_dest"
         echo "entrypoint: $_name skill installed to ~/.claude/skills/"
     elif [ -e "$_dest" ] || [ -L "$_dest" ]; then

container/skills/pia-vpn/SKILL.md

@@ -183,22 +183,34 @@ true in test mode too, and means much less than it sounds like.
 `down` restores `resolv.conf` from its backup (only if that backup still looks
 like a resolver file — restoring a truncated one would leave the container with
 no DNS at all), removes exactly the routes that were added, in reverse order,
-deletes the interface, and shreds the WireGuard private key. It is safe to run
+and deletes the interface. It is safe to run when nothing is up. Confirm
-when nothing is up, and `up` runs it first so a repeat `up` cannot stack state.
+afterwards that the public address is back to the container's own.
-Confirm afterwards that the public address is back to the container's own.
 
-The key deletion is not housekeeping: `/run` is in the container's writable
+`up` calls it too, but only *after* every network fetch has succeeded, so a
-layer, so `docker commit` bakes whatever is there into the project's snapshot
+failed `up` leaves an existing tunnel alone rather than tearing it down to
-image. A key left behind rides that image into every future container.
+report a bad password. From that point on a rollback is armed: if any step of
+the setup fails, the tunnel is torn down rather than left half-configured.
+
+The private key is deleted earlier still — the moment `wg set` has read it,
+while the tunnel is being built. That is not housekeeping: `/run` is in the
+container's writable layer, and recreating or migrating the project runs
+`docker commit` over it *without* tearing the tunnel down first. A key that
+lived for the tunnel's lifetime would be baked into the snapshot image and
+copied forward from then on. The kernel keeps its own copy, so nothing is lost.
 
 ## What this deliberately does not do
 
-- **No killswitch.** Blocking non-tunnel egress needs `iptables`, which is not
+- **No killswitch.** `iptables` *is* in the image, so one is buildable — this
-  in the image, and would cut Claude Code's API traffic whenever the tunnel is
+  is a deliberate omission, not a missing dependency. Blocking non-tunnel egress
-  down. If the user needs guaranteed egress rather than convenient egress, say
+  cuts Claude Code's own API traffic the moment the tunnel drops, which ends the
-  so plainly rather than improvising one — it is a real design decision.
+  session that would otherwise fix it. If the user needs guaranteed egress
+  rather than convenient egress, say so plainly and let them decide, rather than
+  improvising one.
 - **No autostart.** There is no service manager in the container and Triple-C
-  has no start hook, so nothing can re-establish the tunnel automatically.
+  has no start hook, so nothing re-establishes the tunnel on its own. `cron` is
+  in the image and `triple-c-scheduler` runs on it, so a scheduled reconnect is
+  possible if the user wants one — it is just not set up, and a tunnel that
+  reconnects unattended deserves an explicit decision.
 - **Not PIA's desktop client.** `pia-daemon` and `piactl` are installable but
   cannot work headless: the daemon never accepts a client connection without
   the GUI, and `piactl --help` states that connecting requires it. If you find

container/skills/pia-vpn/pia-wg.sh

@@ -27,6 +27,9 @@
 set -euo pipefail
 
 # Not ~/pia-creds: under sudo, HOME is /root.
+# Read by up()'s EXIT trap, which runs after the function's locals are gone.
+SETUP_OK=0
+
 CREDS=${PIA_CREDS:-/home/claude/pia-creds}
 REGION=${PIA_REGION:-us_chicago}
 IFACE=pia0
@@ -88,7 +91,7 @@ preflight() {
 # tunnel captures everything while the exclusions that keep DNS and the Docker
 # host reachable are quietly missing -- and `status` still says "full tunnel".
 add_route() {
-  ip route add "$@" || die "could not add route '$*'. Run 'down' to undo the partial setup."
+  ip route add "$@" || die "could not add route '$*'"
   printf '%s\n' "$*" >> "$STATE/routes"
 }
 
@@ -100,11 +103,6 @@ up() {
   esac
   preflight
 
-  # Always start from a known state. Without this a second `up` overwrites the
-  # saved resolv.conf with PIA's own resolvers, so the later `down` "restores"
-  # those and leaves the container with no working DNS and no way back.
-  down >/dev/null 2>&1 || true
-
   mkdir -p "$STATE"; cd "$STATE"
 
   # `curl -o` creates the file before it knows the request failed, so a plain
@@ -135,6 +133,27 @@ up() {
   sip=$(echo "$srv" | jq -r .ip); scn=$(echo "$srv" | jq -r .cn)
   [ -n "$sip" ] && [ "$sip" != null ] || die "no WireGuard server for region '$REGION'"
 
+  # Only now tear down any previous tunnel. Doing it up front (as an earlier
+  # version did) meant a failed token fetch or an unreachable server list took
+  # a *working* tunnel down with it and silently reverted the container to its
+  # real address, while the error talked about credentials. Everything above
+  # this line can fail; nothing above it has touched the network stack.
+  #
+  # It also still does the job it was added for: clearing a stale resolv.conf
+  # backup so a second `up` cannot save PIA's own resolvers over the real ones.
+  down >/dev/null 2>&1 || true
+
+  # From here on the network stack is being modified, so any failure has to put
+  # it back rather than exit half-configured. `down` is idempotent and restores
+  # routes and resolv.conf exactly.
+  #
+  # EXIT rather than ERR, and a flag rather than the trap's own exit status: an
+  # ERR trap is not inherited by shell functions without `set -E`, so a failure
+  # inside add_route would not fire it, and `die` exits explicitly, which is not
+  # an error and would not fire it either. EXIT catches both.
+  SETUP_OK=0
+  trap '[ "$SETUP_OK" = 1 ] || { echo "pia-wg: setup failed - rolling back" >&2; down >/dev/null 2>&1; }' EXIT
+
   # umask, not a later chmod: the file is created under the inherited 0022
   # otherwise, so the key is world-readable for the moment in between.
   ( umask 077; priv=$(wg genkey); printf '%s' "$priv" > wg.priv )
@@ -159,6 +178,12 @@ up() {
     peer "$(echo "$resp" | jq -r .server_key)" \
     endpoint "$(echo "$resp" | jq -r .server_ip):$(echo "$resp" | jq -r .server_port)" \
     allowed-ips 0.0.0.0/0 persistent-keepalive 25
+  # The kernel holds the key from here, so the file has no reason to outlive
+  # this line -- and every reason not to: /run is in the writable layer, and a
+  # recreate or migrate runs `docker commit` over it without tearing the tunnel
+  # down first, baking the key into the project's snapshot image. `down` also
+  # removes it, for the case where `up` never got this far.
+  rm -f wg.priv
   ip addr add "$(echo "$resp" | jq -r .peer_ip)/32" dev "$IFACE"
   ip link set "$IFACE" up
 
@@ -201,7 +226,18 @@ up() {
     echo "test route only: 1.1.1.1 goes via PIA, everything else unchanged"
   fi
 
-  sleep 2
+  # A tunnel with no handshake still routes -- into a black hole. Without this
+  # `up --full` would exit 0 having pointed all traffic *and* resolv.conf at a
+  # peer that never answered, and `status` would print "mode: full tunnel".
+  local waited=0
+  until [ "$(wg show "$IFACE" latest-handshakes | awk '{print $2; exit}')" != 0 ]; do
+    waited=$((waited + 1))
+    [ "$waited" -lt 20 ] || die "no handshake from $REGION after 10s - rolled back"
+    sleep 0.5
+  done
+
+  SETUP_OK=1
+  trap - EXIT
   status
 }
 
@@ -242,6 +278,10 @@ TRACE_DIRECT=https://1.0.0.1/cdn-cgi/trace
 exit_ip() { curl -s -m 20 "$1" | sed -n 's/^ip=//p'; }
 
 status() {
+  # `wg show` needs root; `ip route`/`ip link` do not. Without this guard an
+  # unprivileged run prints "no tunnel up" and then "mode: full tunnel" in the
+  # same breath, and an agent reading the first line re-runs `up`.
+  [ "$(id -u)" = 0 ] || die "run with sudo"
   wg show "$IFACE" 2>/dev/null | grep -E "latest handshake|transfer" || echo "no tunnel up"
 
   # Resolve a name, not an IP literal. A curl to 1.1.1.1 succeeds while DNS is
@@ -274,5 +314,5 @@ case "${1:-}" in
   up) shift; up "${1:-}" ;;
   down) down ;;
   status) status ;;
-  *) sed -n '2,27p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;;
+  *) sed -n '2,26p' "$0" | sed 's/^# \{0,1\}//'; exit 1 ;;
 esac