Fix nine defects in the push/sync path found in review

The worst was fatal: push passed --skip-prod-check to upload-prebuilt, which forwards every argument to sign, promote and update as well, and sign rejects unknown options. Every non-dry-run push and deploy would have uploaded and verified its artifacts and then failed before signing. upload-prebuilt now routes publishing flags to sync alone. The partial-tree guard was weaker than it looked: - it counted archive files locally against package names in the remote database, and this tree keeps two versions per package, so a checkout with a spare version of half the repository could pass while still hiding hundreds of packages. It now compares package-name sets and lists what would be hidden. - it treated any unreadable remote as an empty one, so an auth failure or a corrupt database disabled it. Only rclone's "directory not found" now counts as a fresh mirror; every other failure aborts. Also: - sync had no set -e, so a failed package upload fell through to publishing the database, advertising packages that were never uploaded. Each transfer is now checked before the next step. - --package with no names silently meant "every package", which under --yes could publish everything from one unset variable in a script. - push now refuses to run when the host has packages staged from an earlier failure, since publishing would sign and promote those too. - epoch versions contain a colon, which rsync reads as host:path, so no package with an epoch could be transferred. Sources are ./-prefixed. - remote paths are quoted for the remote shell. - sync spun forever on a missing option value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-12 03:39:55 -07:00
parent c5f5f1c12c
commit fd9c078bf2
5 changed files with 192 additions and 33 deletions
@@ -13,6 +13,7 @@ source "$BUILD_ROOT/helpers/message-helpers.sh"
 source "$BUILD_ROOT/helpers/paths.sh"

 PACKAGES=()
+PACKAGE_FLAG_GIVEN=false
 HOST=""
 REMOTE_ROOT=""
 DRY_RUN=false
@@ -38,8 +39,9 @@ while [[ $# -gt 0 ]]; do
    ;;
  --package)
    shift
+    PACKAGE_FLAG_GIVEN=true
    while [[ $# -gt 0 && ! "$1" =~ ^-- ]]; do
-      PACKAGES+=("$1")
+      [[ -n "$1" ]] && PACKAGES+=("$1")
      shift
    done
    ;;
@@ -86,6 +88,11 @@ while [[ $# -gt 0 ]]; do
  esac
 done

+if [[ "$PACKAGE_FLAG_GIVEN" == true && ${#PACKAGES[@]} -eq 0 ]]; then
+  print_error "--package requires at least one package name"
+  exit 1
+fi
+
 echo ""
 print_info "This will:"
 echo "  1. Build packages locally"
@@ -17,8 +17,10 @@ HOST=""
 REMOTE_ROOT="/root/omarchy-pkgs"
 CREDENTIALS="/root/.omarchy/build-credentials"
 PACKAGES=""
+PACKAGE_FLAG_GIVEN=false
 DRY_RUN=false
 ASSUME_YES=false
+INCLUDE_STAGED=false

 print_header "Push Build to Host"

@@ -40,9 +42,9 @@ while [[ $# -gt 0 ]]; do
    ;;
  --package)
    shift
-    PACKAGES=""
+    PACKAGE_FLAG_GIVEN=true
    while [[ $# -gt 0 && ! "$1" =~ ^-- ]]; do
-      PACKAGES="$PACKAGES $1"
+      [[ -n "$1" ]] && PACKAGES="$PACKAGES $1"
      shift
    done
    PACKAGES="${PACKAGES# }"
@@ -63,6 +65,10 @@ while [[ $# -gt 0 ]]; do
    ASSUME_YES=true
    shift
    ;;
+  --include-staged)
+    INCLUDE_STAGED=true
+    shift
+    ;;
  -h | --help)
    echo "Usage: $0 [OPTIONS]"
    echo ""
@@ -77,6 +83,7 @@ while [[ $# -gt 0 ]]; do
    echo "  --remote-root <path>   Repository path on the host (default: $REMOTE_ROOT)"
    echo "  --dry-run              Show what would be pushed, transfer nothing"
    echo "  -y, --yes              Do not ask for confirmation"
+    echo "  --include-staged       Publish packages already staged on the host too"
    echo "  -h, --help             Show this help message"
    echo ""
    echo "Typical use:"
@@ -122,6 +129,14 @@ fi
 # is rebuilt there, so neither should ride along.
 mapfile -t ALL_FILES < <(cd "$BUILD_OUTPUT_DIR" && ls -1 *.pkg.tar.* 2>/dev/null | grep -v '\.sig$' || true)

+# "--package" with nothing after it, or with an empty variable, must not quietly
+# widen to every artifact — that is the difference between shipping one package
+# and shipping whatever else happens to be lying around.
+if [[ "$PACKAGE_FLAG_GIVEN" == true && -z "$PACKAGES" ]]; then
+  print_error "--package requires at least one package name"
+  exit 1
+fi
+
 FILES=()
 if [[ -z "$PACKAGES" ]]; then
  FILES=("${ALL_FILES[@]}")
@@ -198,24 +213,61 @@ fi

 # --- transfer ----------------------------------------------------------------

+# Remote paths are interpolated into shell command strings, so quote them for the
+# remote shell rather than trusting them to contain nothing surprising.
+q_remote_root=$(printf '%q' "$REMOTE_ROOT")
+q_remote_output=$(printf '%q' "$REMOTE_BUILD_OUTPUT")
+q_credentials=$(printf '%q' "$CREDENTIALS")
+
 print_info "Checking host..."
-if ! ssh "$HOST" "test -d $REMOTE_ROOT"; then
+if ! ssh "$HOST" "test -d $q_remote_root"; then
  print_error "Repository not found on host: $REMOTE_ROOT"
  print_warning "Pass --remote-root if it lives elsewhere"
  exit 1
 fi
-ssh "$HOST" "mkdir -p $REMOTE_BUILD_OUTPUT"
+ssh "$HOST" "mkdir -p $q_remote_output"
+
+# upload-prebuilt signs and promotes everything in the host's build-output, not
+# just what we are about to send. Anything already sitting there — typically the
+# leftovers of an earlier failed push — would ride along unnoticed.
+staged=$(ssh "$HOST" "cd $q_remote_output && ls -1 *.pkg.tar.* 2>/dev/null | grep -v '\.sig\$' || true")
+unexpected=""
+if [[ -n "$staged" ]]; then
+  while IFS= read -r remote_file; do
+    [[ -z "$remote_file" ]] && continue
+    for file in "${FILES[@]}"; do
+      [[ "$remote_file" == "$file" ]] && continue 2
+    done
+    unexpected+="$remote_file"$'\n'
+  done <<<"$staged"
+fi
+
+if [[ -n "$unexpected" && "$INCLUDE_STAGED" != true ]]; then
+  print_error "The host already has staged packages this push did not build:"
+  echo ""
+  echo "$unexpected" | grep -v '^$' | sed 's/^/  /'
+  echo ""
+  echo "Publishing signs and promotes everything in $REMOTE_BUILD_OUTPUT, so these"
+  echo "would be published too. They are usually left over from a failed push."
+  echo ""
+  echo "Remove them on the host, or pass --include-staged to publish them as well."
+  exit 1
+fi
 print_success "Host ready"
 echo ""

 print_info "Uploading packages..."
-(cd "$BUILD_OUTPUT_DIR" && rsync -a --info=progress2 --partial "${FILES[@]}" "$HOST:$REMOTE_BUILD_OUTPUT/")
+# Prefix with ./ so rsync does not read an epoch's colon (spotify-1:1.2.3-...)
+# as a host:path separator.
+rsync_sources=()
+for file in "${FILES[@]}"; do rsync_sources+=("./$file"); done
+(cd "$BUILD_OUTPUT_DIR" && rsync -a --info=progress2 --partial "${rsync_sources[@]}" "$HOST:$REMOTE_BUILD_OUTPUT/")
 print_success "Upload complete"
 echo ""

 print_info "Verifying checksums..."
 local_sums=$(cd "$BUILD_OUTPUT_DIR" && sha256sum "${FILES[@]}" | sort)
-remote_sums=$(ssh "$HOST" "cd $REMOTE_BUILD_OUTPUT && sha256sum $(printf '%q ' "${FILES[@]}")" | sort)
+remote_sums=$(ssh "$HOST" "cd $q_remote_output && sha256sum $(printf '%q ' "${FILES[@]}")" | sort)
 if [[ "$local_sums" != "$remote_sums" ]]; then
  print_error "Checksum mismatch after upload"
  diff <(echo "$local_sums") <(echo "$remote_sums") || true
@@ -228,7 +280,7 @@ echo ""

 print_info "Publishing on $HOST (sign -> promote -> update -> sync)..."
 echo ""
-if ! ssh "$HOST" "source $CREDENTIALS && cd $REMOTE_ROOT && bin/upload-prebuilt --mirror $MIRROR --arch $ARCH --skip-prod-check"; then
+if ! ssh "$HOST" "source $q_credentials && cd $q_remote_root && bin/upload-prebuilt --mirror $(printf '%q' "$MIRROR") --arch $(printf '%q' "$ARCH") --skip-prod-check"; then
  print_error "Remote publish failed"
  print_warning "The uploaded packages are still in $REMOTE_BUILD_OUTPUT on $HOST"
  exit 1
@@ -14,20 +14,32 @@ PRUNE=false
 # Print header
 print_header "Sync Repository to Remote"

+# This script has no `set -e`, so a `shift 2` past the end of the argument list
+# fails without consuming anything and the loop spins forever. Check first.
+require_value() {
+  if [[ $# -lt 2 || -z "$2" ]]; then
+    print_error "Option $1 requires a value"
+    exit 1
+  fi
+}
+
 # Parse arguments
 while [[ $# -gt 0 ]]; do
  case $1 in
  --arch)
+    require_value "$@"
    ARCH="$2"
    update_arch_paths
    shift 2
    ;;
  --mirror)
+    require_value "$@"
    MIRROR="$2"
    update_arch_paths
    shift 2
    ;;
  --remote)
+    require_value "$@"
    REMOTE="$2"
    shift 2
    ;;
@@ -93,26 +105,75 @@ print_info "Syncing to: $REMOTE/$DESTINATION_DIRECTORY"
 # from this tree alone. Publishing one built from a partial tree hides every
 # package it does not know about, even though the files are still on the remote.
 # Refuse to shrink the package list unless that is the stated intent.
-LOCAL_COUNT=$(ls -1 "$REPO_DIR"/*.pkg.tar.* 2>/dev/null | grep -vc '\.sig$' || true)
-# bsdtar, not tar: the database is compressed and GNU tar will not detect that
-# on a pipe. repo-add has used both gzip and zstd, so let libarchive decide.
-REMOTE_COUNT=$(rclone cat "$REMOTE/$DESTINATION_DIRECTORY/omarchy.db" --s3-no-head 2>/dev/null |
-  bsdtar -tf - 2>/dev/null | sed 's|/.*||' | sort -u | grep -c . || true)
+#
+# Compare package names, not file counts: this tree keeps several versions of
+# each package (bin/repo clean --keep 2) while the database carries one entry per
+# name, so counting files would compare unrelated quantities and let a partial
+# tree through whenever its spare versions made up the difference.
+strip_version() { sed -E 's/-[^-]+-[^-]+-[^-]+\.pkg\.tar\.[^.]+$//'; }

-if [[ "${REMOTE_COUNT:-0}" -gt 0 && "${LOCAL_COUNT:-0}" -lt "$REMOTE_COUNT" && "$PRUNE" != true ]]; then
-  print_error "Local tree has $LOCAL_COUNT package(s); the remote database lists $REMOTE_COUNT"
+LOCAL_NAMES=$(ls -1 "$REPO_DIR" 2>/dev/null | grep -v '\.sig$' | grep '\.pkg\.tar\.' |
+  strip_version | sort -u)
+
+# Distinguish "no repository there yet" from "cannot read the repository". Only
+# the first is safe to treat as an empty remote; failing open on a credential or
+# network error is how a partial database reaches production.
+REMOTE_LISTING=$(rclone lsf "$REMOTE/$DESTINATION_DIRECTORY/" --s3-no-head 2>&1)
+RCLONE_STATUS=$?
+
+# rclone exit 3 is "directory not found", which is what a mirror that has never
+# been published looks like. Every other failure means the remote could not be
+# read, and an unread remote must not be mistaken for an empty one.
+if [[ $RCLONE_STATUS -eq 3 ]]; then
+  REMOTE_LISTING=""
+elif [[ $RCLONE_STATUS -ne 0 ]]; then
+  print_error "Cannot read the remote repository (rclone exit $RCLONE_STATUS)"
+  echo "$REMOTE_LISTING"
  echo ""
-  echo "Publishing this database would hide the $((REMOTE_COUNT - LOCAL_COUNT)) package(s)"
-  echo "missing from $REPO_DIR."
-  echo ""
-  echo "To publish packages built on this machine, push them to the build host,"
-  echo "which holds the complete repository:"
-  echo "  bin/repo push --mirror $MIRROR --arch $ARCH"
-  echo ""
-  echo "If shrinking the repository is genuinely what you want, pass --prune."
+  echo "Refusing to sync: an unreadable remote cannot be checked for packages"
+  echo "this tree would hide."
  exit 1
 fi

+if grep -qx 'omarchy\.db' <<<"$REMOTE_LISTING"; then
+  # bsdtar, not tar: the database is compressed and GNU tar will not detect that
+  # on a pipe. repo-add has used both gzip and zstd, so let libarchive decide.
+  REMOTE_DB_FILE=$(mktemp)
+  trap 'rm -f "$REMOTE_DB_FILE"' EXIT
+  rclone cat "$REMOTE/$DESTINATION_DIRECTORY/omarchy.db" --s3-no-head >"$REMOTE_DB_FILE" 2>/dev/null
+  REMOTE_NAMES=$(bsdtar -tf "$REMOTE_DB_FILE" 2>/dev/null | sed 's|/.*||' |
+    sed -E 's/-[^-]+-[^-]+$//' | sort -u)
+
+  if [[ -z "$REMOTE_NAMES" ]]; then
+    print_error "The remote database exists but could not be read"
+    echo ""
+    echo "Refusing to sync rather than assume the remote is empty. Check that"
+    echo "bsdtar is installed and that omarchy.db is not corrupt."
+    exit 1
+  fi
+
+  HIDDEN=$(comm -23 <(echo "$REMOTE_NAMES") <(echo "$LOCAL_NAMES"))
+  HIDDEN_COUNT=$(grep -c '' <<<"$HIDDEN")
+  [[ -z "$HIDDEN" ]] && HIDDEN_COUNT=0
+
+  if [[ "$HIDDEN_COUNT" -gt 0 && "$PRUNE" != true ]]; then
+    print_error "$HIDDEN_COUNT package(s) in the remote database are missing from this tree"
+    echo ""
+    echo "$HIDDEN" | head -10 | sed 's/^/  /'
+    [[ "$HIDDEN_COUNT" -gt 10 ]] && echo "  ... and $((HIDDEN_COUNT - 10)) more"
+    echo ""
+    echo "Publishing a database built here would hide them, even though their"
+    echo "files remain on the mirror."
+    echo ""
+    echo "To publish packages built on this machine, push them to the build host,"
+    echo "which holds the complete repository:"
+    echo "  bin/repo push --mirror $MIRROR --arch $ARCH"
+    echo ""
+    echo "If shrinking the repository is genuinely what you want, pass --prune."
+    exit 1
+  fi
+fi
+
 # Upload packages first, database last, so the remote never advertises a package
 # it does not yet have.
 #
@@ -133,27 +194,40 @@ if [[ "$PRUNE" == true ]]; then
    fi
  fi
  print_info "Syncing packages (with prune)..."
-  rclone sync "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
+  if ! rclone sync "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
    --s3-no-head \
    --exclude "omarchy.db*" \
    --exclude "omarchy.files*" \
    --ignore-existing \
-    --copy-links --delete-after -v
+    --copy-links --delete-after -v; then
+    print_error "Package sync failed — not publishing the database"
+    exit 1
+  fi
 else
  print_info "Syncing packages..."
-  rclone copy "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
+  if ! rclone copy "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
    --s3-no-head \
    --exclude "omarchy.db*" \
    --exclude "omarchy.files*" \
    --ignore-existing \
-    --copy-links -v
+    --copy-links -v; then
+    print_error "Package upload failed — not publishing the database"
+    print_warning "The remote database still describes the previous contents, so"
+    print_warning "the repository is unchanged and consistent."
+    exit 1
+  fi
 fi

 # Then sync database files last to ensure repository integrity
 print_info "Updating repository database..."
-rclone copy "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
+if ! rclone copy "$REPO_DIR" "$REMOTE/$DESTINATION_DIRECTORY" \
  --s3-no-head \
  --include "omarchy.*" \
-  --checksum --copy-links -v
+  --checksum --copy-links -v; then
+  print_error "Database upload failed"
+  print_warning "Packages were uploaded but the database still describes the"
+  print_warning "previous contents. Re-run sync to finish publishing them."
+  exit 1
+fi

 print_success "Sync complete!"
@@ -5,7 +5,28 @@ set -e

 SCRIPT_DIR=$(realpath "${BASH_SOURCE[0]%/*}")

-"$SCRIPT_DIR/repo" sign "$@"
-"$SCRIPT_DIR/repo" promote "$@"
-"$SCRIPT_DIR/repo" update "$@"
-"$SCRIPT_DIR/repo" sync "$@"
+# Only sync understands the publishing flags; sign, promote and update reject
+# unknown options outright, so they cannot be forwarded blindly.
+COMMON_ARGS=()
+SYNC_ARGS=()
+while [[ $# -gt 0 ]]; do
+  case $1 in
+  --skip-prod-check | --prune)
+    SYNC_ARGS+=("$1")
+    shift
+    ;;
+  --remote)
+    SYNC_ARGS+=("$1" "$2")
+    shift 2
+    ;;
+  *)
+    COMMON_ARGS+=("$1")
+    shift
+    ;;
+  esac
+done
+
+"$SCRIPT_DIR/repo" sign "${COMMON_ARGS[@]}"
+"$SCRIPT_DIR/repo" promote "${COMMON_ARGS[@]}"
+"$SCRIPT_DIR/repo" update "${COMMON_ARGS[@]}"
+"$SCRIPT_DIR/repo" sync "${COMMON_ARGS[@]}" "${SYNC_ARGS[@]}"