Skip to content

fix: simplify cliproxyapi backup spec preprocessing #457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shunkakinoki merged 1 commit into from
Dec 27, 2025
Merged

fix: simplify cliproxyapi backup spec preprocessing #457

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions spec/cliproxyapi_backup_spec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,37 @@
Describe 'cliproxyapi backup scripts'
SCRIPTS_DIR="$PWD/home-manager/services/cliproxyapi/scripts"

# Preprocess scripts once at describe-time (not in setup)
# This ensures the preprocessed paths are available before any tests run
__PREPROCESSED_DIR=$(mktemp -d)

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary directory created at describe-time is not cleaned up if the test suite is interrupted before reaching the AfterAll hook. This could lead to accumulating temporary directories in /tmp. Consider using a trap or ensuring the cleanup happens even on early exit or test failure.

Suggested change
__PREPROCESSED_DIR=$(mktemp -d)
__PREPROCESSED_DIR=$(mktemp -d)
__cleanup_preprocessed_dir() {
if [ -n "$__PREPROCESSED_DIR" ] && [ -d "$__PREPROCESSED_DIR" ]; then
rm -rf "$__PREPROCESSED_DIR"
fi
}
# Ensure the preprocessed directory is cleaned up even if the test suite is interrupted
trap '__cleanup_preprocessed_dir' EXIT INT TERM

Copilot uses AI. Check for mistakes.
__BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh"
__BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh"

# Preprocess backup-auth.sh
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sed replacement for @Sed@ placeholder is unnecessary here because the backup-auth.sh script does not use @Sed@. Looking at the actual script content, only @aws@ and @rsync@ placeholders are present in backup-auth.sh. Including unnecessary replacements can be confusing and may mask issues if the wrong script is accidentally preprocessed.

Suggested change
-e 's|@sed@|sed|g' \

Copilot uses AI. Check for mistakes.
"$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"
chmod +x "$__BACKUP_AUTH_SCRIPT"

# Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
Comment on lines +24 to +27

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sed replacements for @aws@, @rsync@, and @Sed@ are unnecessary for backup-and-recover.sh. Looking at the actual script content, it only uses @bash@ and @backupAuthScript@ placeholders. Including unnecessary replacements makes the preprocessing logic harder to maintain and understand.

Suggested change
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
-e 's|@bash@|bash|g' \

Copilot uses AI. Check for mistakes.
-e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \
"$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"
chmod +x "$__BACKUP_RECOVER_SCRIPT"
Comment on lines +9 to +30

@gemini-code-assist gemini-code-assist Bot Dec 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve maintainability and reduce code duplication, you can define the common sed arguments in an array and reuse it. Also, consider using mktemp -d -t <template> to create temporary directories with a more descriptive name, which can aid in debugging.

Suggested change
__PREPROCESSED_DIR=$(mktemp -d)
__BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh"
__BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh"
# Preprocess backup-auth.sh
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
"$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"
chmod +x "$__BACKUP_AUTH_SCRIPT"
# Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
-e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \
"$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"
chmod +x "$__BACKUP_RECOVER_SCRIPT"
__PREPROCESSED_DIR=$(mktemp -d -t cliproxy-spec-XXXXXXXX)
__BACKUP_AUTH_SCRIPT="$__PREPROCESSED_DIR/backup-auth.sh"
__BACKUP_RECOVER_SCRIPT="$__PREPROCESSED_DIR/backup-and-recover.sh"
COMMON_SED_ARGS=(
-e 's|@aws@|aws|g'
-e 's|@rsync@|rsync|g'
-e 's|@bash@|bash|g'
-e 's|@sed@|sed|g'
)
# Preprocess backup-auth.sh
sed "${COMMON_SED_ARGS[@]}" "$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"
chmod +x "$__BACKUP_AUTH_SCRIPT"
# Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh
sed "${COMMON_SED_ARGS[@]}" \
-e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \
"$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"
chmod +x "$__BACKUP_RECOVER_SCRIPT"

Comment on lines +14 to +30

Copilot AI Dec 27, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preprocessing sed commands do not check for errors. If the source script files don't exist or if writing to the output fails, the test will continue with potentially empty or corrupted script files, leading to confusing test failures. Consider adding error checking after these preprocessing operations.

Suggested change
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
"$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"
chmod +x "$__BACKUP_AUTH_SCRIPT"
# Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh
sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
-e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \
"$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"
chmod +x "$__BACKUP_RECOVER_SCRIPT"
if ! sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
"$SCRIPTS_DIR/backup-auth.sh" >"$__BACKUP_AUTH_SCRIPT"; then
echo "Error: failed to preprocess backup-auth.sh from $SCRIPTS_DIR" >&2
exit 1
fi
if ! chmod +x "$__BACKUP_AUTH_SCRIPT"; then
echo "Error: failed to make preprocessed backup-auth.sh executable at $__BACKUP_AUTH_SCRIPT" >&2
exit 1
fi
# Preprocess backup-and-recover.sh with path to preprocessed backup-auth.sh
if ! sed \
-e 's|@aws@|aws|g' \
-e 's|@rsync@|rsync|g' \
-e 's|@bash@|bash|g' \
-e 's|@sed@|sed|g' \
-e "s|@backupAuthScript@|$__BACKUP_AUTH_SCRIPT|g" \
"$SCRIPTS_DIR/backup-and-recover.sh" >"$__BACKUP_RECOVER_SCRIPT"; then
echo "Error: failed to preprocess backup-and-recover.sh from $SCRIPTS_DIR" >&2
exit 1
fi
if ! chmod +x "$__BACKUP_RECOVER_SCRIPT"; then
echo "Error: failed to make preprocessed backup-and-recover.sh executable at $__BACKUP_RECOVER_SCRIPT" >&2
exit 1
fi

Copilot uses AI. Check for mistakes.

Describe 'backup-auth.sh'

setup() {
mock_bin_setup aws rsync
TEMP_HOME=$(mktemp -d)
mkdir -p "$TEMP_HOME/.cli-proxy-api/objectstore/auths"
# Preprocess script to replace @aws@ and @rsync@ placeholders
SCRIPT=$(nix_script_preprocess "$SCRIPTS_DIR/backup-auth.sh")

# Unset objectstore credentials to ensure clean test environment
unset OBJECTSTORE_ACCESS_KEY
Expand All @@ -22,14 +45,13 @@ setup() {
cleanup() {
rm -rf "$TEMP_HOME"
mock_bin_cleanup
nix_script_cleanup
}

Before 'setup'
After 'cleanup'

It 'pulls from R2 but skips push when auth directory is empty'
When run bash -c 'env HOME="'"$TEMP_HOME"'" OBJECTSTORE_ACCESS_KEY=key OBJECTSTORE_SECRET_KEY=secret OBJECTSTORE_ENDPOINT=https://example.com bash '"$SCRIPT"' 2>&1; cat "$MOCK_LOG" 2>/dev/null || true'
When run bash -c 'HOME="'"$TEMP_HOME"'" OBJECTSTORE_ACCESS_KEY=key OBJECTSTORE_SECRET_KEY=secret OBJECTSTORE_ENDPOINT=https://example.com bash "'"$__BACKUP_AUTH_SCRIPT"'" 2>&1; cat "$MOCK_LOG" 2>/dev/null || true'
The status should be success
# Should pull from R2 auths/ and backup/auths/
The output should include 's3://cliproxyapi/auths/'
Expand All @@ -40,7 +62,7 @@ End

It 'calls aws s3 sync when auth files exist'
touch "$TEMP_HOME/.cli-proxy-api/objectstore/auths/test-auth.json"
When run bash -c 'env HOME="'"$TEMP_HOME"'" OBJECTSTORE_ACCESS_KEY=key OBJECTSTORE_SECRET_KEY=secret OBJECTSTORE_ENDPOINT=https://example.com bash '"$SCRIPT"' 2>&1; cat "$MOCK_LOG"'
When run bash -c 'HOME="'"$TEMP_HOME"'" OBJECTSTORE_ACCESS_KEY=key OBJECTSTORE_SECRET_KEY=secret OBJECTSTORE_ENDPOINT=https://example.com bash "'"$__BACKUP_AUTH_SCRIPT"'" 2>&1; cat "$MOCK_LOG"'
The status should be success
The output should include 's3 sync'
The output should include 's3://cliproxyapi/backup/auths/'
Expand All @@ -55,9 +77,6 @@ setup() {
mkdir -p "$TEMP_HOME/.cli-proxy-api/objectstore/auths"
mkdir -p "$TEMP_HOME/dotfiles"

# Preprocess script with its dependencies
SCRIPT=$(nix_script_preprocess_with_deps "$SCRIPTS_DIR/backup-and-recover.sh" backup-auth.sh)

# Create .env with test credentials
cat >"$TEMP_HOME/dotfiles/.env" <<'ENV'
OBJECTSTORE_ACCESS_KEY=test_key
Expand All @@ -73,18 +92,23 @@ ENV
cleanup() {
rm -rf "$TEMP_HOME"
mock_bin_cleanup
nix_script_cleanup
}

Before 'setup'
After 'cleanup'

It 'sources .env and runs backup script'
When run bash -c 'env HOME="'"$TEMP_HOME"'" bash '"$SCRIPT"' 2>&1'
When run bash -c 'HOME="'"$TEMP_HOME"'" bash "'"$__BACKUP_RECOVER_SCRIPT"'" 2>&1'
The status should be success
The output should include 'Starting backup'
The output should include 'Backup complete'
End
End

# Cleanup preprocessed scripts at end
cleanup_preprocessed() {
rm -rf "$__PREPROCESSED_DIR"
}
Comment on lines +109 to +111

@gemini-code-assist gemini-code-assist Bot Dec 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

For improved safety, it's a good practice to add a check to ensure that __PREPROCESSED_DIR is a non-empty string and an existing directory before attempting to remove it recursively. This prevents accidental deletion of unintended files or directories if the variable were to be unexpectedly empty or unset.

Suggested change
cleanup_preprocessed() {
rm -rf "$__PREPROCESSED_DIR"
}
cleanup_preprocessed() {
if [[ -n "$__PREPROCESSED_DIR" && -d "$__PREPROCESSED_DIR" ]]; then
rm -rf "$__PREPROCESSED_DIR"
fi
}

AfterAll 'cleanup_preprocessed'

End
Loading