feat(cliproxyapi): ensure required auth files exist to prevent bootstrap failures - #488
Conversation
…rap failures Add step to backup-auth.sh that ensures required auth files are present, restoring from dotfiles backup if available or creating empty placeholders to prevent bootstrap failures when files are missing.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThese changes enhance bootstrap resilience for the cliproxyapi service by introducing a REQUIRED_FILES array in backup-auth.sh that ensures critical authentication files exist, with fallback creation from backup or placeholder, and adding CCS_AUTH_DIR syncing in start.sh to propagate locally-created tokens before downstream syncing. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DREnsures required authentication files for What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a mechanism to ensure required authentication files exist before starting the cliproxyapi service, preventing bootstrap failures. The changes in backup-auth.sh introduce a new step to check for these files, restore them from a dotfiles backup if available, or create empty placeholders as a fallback. My review identifies a couple of areas for improvement in the script. The list of required files contains a hardcoded filename, which could be made configurable via an environment variable for better reusability. Additionally, the new logic for restoring files has a subtle redundancy on macOS due to a previous step; I've suggested a refactoring to improve clarity. Overall, the changes effectively address the problem of bootstrap failures due to missing auth files.
| AUTH_DIR="$CONFIG_DIR/objectstore/auths" | ||
| CCS_AUTH_DIR="$HOME/.ccs/cliproxy/auth" | ||
| DOTFILES_AUTH_DIR="$HOME/dotfiles/objectstore/auths" | ||
| REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") |
There was a problem hiding this comment.
The required auth filename is hardcoded. This reduces the script's reusability and makes it difficult to configure for different users or environments. It's better practice to allow this to be configured via an environment variable, with the current value as a default.
| REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") | |
| read -r -a REQUIRED_FILES <<< "${REQUIRED_AUTH_FILES:-"shunkakinoki@gmail.com-shunkakinoki.json"}" |
| for fname in "${REQUIRED_FILES[@]}"; do | ||
| target="$AUTH_DIR/$fname" | ||
| if [ ! -f "$target" ]; then | ||
| # Prefer dotfiles copy if present | ||
| if [ -f "$DOTFILES_AUTH_DIR/$fname" ]; then | ||
| @rsync@ -a "$DOTFILES_AUTH_DIR/$fname" "$target" | ||
| echo "✅ Restored missing $fname from dotfiles backup" >&2 | ||
| else | ||
| echo "{}" >"$target" | ||
| chmod 600 "$target" | ||
| echo "⚠️ Created empty placeholder for $fname (no backup found)" >&2 | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
The logic in this loop has a subtle platform-dependent behavior due to its interaction with STEP 3, which runs only on macOS. On macOS, the check for a backup in DOTFILES_AUTH_DIR inside this loop is redundant because STEP 3 would have already copied the file. This can be confusing for future maintainers. Refactoring this block to flatten the conditional logic and add comments explaining the behavior would improve clarity.
| for fname in "${REQUIRED_FILES[@]}"; do | |
| target="$AUTH_DIR/$fname" | |
| if [ ! -f "$target" ]; then | |
| # Prefer dotfiles copy if present | |
| if [ -f "$DOTFILES_AUTH_DIR/$fname" ]; then | |
| @rsync@ -a "$DOTFILES_AUTH_DIR/$fname" "$target" | |
| echo "✅ Restored missing $fname from dotfiles backup" >&2 | |
| else | |
| echo "{}" >"$target" | |
| chmod 600 "$target" | |
| echo "⚠️ Created empty placeholder for $fname (no backup found)" >&2 | |
| fi | |
| fi | |
| done | |
| for fname in "${REQUIRED_FILES[@]}"; do | |
| target="$AUTH_DIR/$fname" | |
| if [ -f "$target" ]; then | |
| continue | |
| fi | |
| # If file is missing, try to restore from dotfiles backup. | |
| # This check is mainly for non-macOS systems, as STEP 3 handles this for macOS. | |
| dotfiles_backup_path="$DOTFILES_AUTH_DIR/$fname" | |
| if [ -f "$dotfiles_backup_path" ]; then | |
| @rsync@ -a "$dotfiles_backup_path" "$target" | |
| echo "✅ Restored missing $fname from dotfiles backup" >&2 | |
| else | |
| # If no backup is found, create an empty placeholder to prevent bootstrap failures. | |
| echo "{}" >"$target" | |
| chmod 600 "$target" | |
| echo "⚠️ Created empty placeholder for $fname (no backup found)" >&2 | |
| fi | |
| done |
There was a problem hiding this comment.
Pull request overview
This PR enhances the backup-auth.sh script to prevent bootstrap failures by ensuring that required authentication files exist before the cliproxyapi service starts. The change adds a new STEP 4 that verifies the presence of critical auth files and creates them if missing, either by restoring from dotfiles backup or creating empty placeholders.
Key Changes:
- Added
REQUIRED_FILESarray to track authentication files that must exist - Implemented STEP 4 to verify and restore/create required auth files
- Enhanced logging for missing file scenarios
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # STEP 4: Ensure required auth files exist to avoid bootstrap failures | ||
| for fname in "${REQUIRED_FILES[@]}"; do | ||
| target="$AUTH_DIR/$fname" | ||
| if [ ! -f "$target" ]; then | ||
| # Prefer dotfiles copy if present | ||
| if [ -f "$DOTFILES_AUTH_DIR/$fname" ]; then | ||
| @rsync@ -a "$DOTFILES_AUTH_DIR/$fname" "$target" | ||
| echo "✅ Restored missing $fname from dotfiles backup" >&2 | ||
| else | ||
| echo "{}" >"$target" | ||
| chmod 600 "$target" | ||
| echo "⚠️ Created empty placeholder for $fname (no backup found)" >&2 | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
The logic in STEP 4 duplicates part of STEP 3's functionality. STEP 3 already syncs files from DOTFILES_AUTH_DIR using rsync with --ignore-existing, which would have restored missing files if they existed. STEP 4 then checks again and potentially copies the same files. This creates redundant file operations for the same source.
Consider whether STEP 4 should only handle the placeholder creation case, since STEP 3 already handles restoration from dotfiles backup.
| AUTH_DIR="$CONFIG_DIR/objectstore/auths" | ||
| CCS_AUTH_DIR="$HOME/.ccs/cliproxy/auth" | ||
| DOTFILES_AUTH_DIR="$HOME/dotfiles/objectstore/auths" | ||
| REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") |
There was a problem hiding this comment.
The email address is hardcoded directly in the REQUIRED_FILES array. This makes the script less maintainable and ties it to a specific user. Consider moving this to a configuration variable or environment variable, especially since this script is meant to prevent bootstrap failures in a more general way.
Alternatively, if this is intentionally user-specific for this repository, consider adding a comment explaining why this particular file is required.
| REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") | |
| # Default required auth files; can be overridden via REQUIRED_AUTH_FILES (space-separated) | |
| # The default file is user-specific and kept for backwards compatibility. | |
| DEFAULT_REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") | |
| if [ -n "${REQUIRED_AUTH_FILES:-}" ]; then | |
| # Parse space-separated REQUIRED_AUTH_FILES into REQUIRED_FILES array | |
| read -r -a REQUIRED_FILES <<< "${REQUIRED_AUTH_FILES}" | |
| else | |
| REQUIRED_FILES=("${DEFAULT_REQUIRED_FILES[@]}") | |
| fi |
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of 9c04d4c...2097f57
Analysis
-
Backup Integrity Risk: The script can propagate empty placeholder credentials back to cloud storage (R2), potentially overwriting legitimate secrets with synthetic data.
-
Reduced System Observability: Creating placeholder files instead of failing explicitly hides bootstrap failures, violating the fail-fast principle and masking real issues.
-
Tight Coupling: Hardcoded tenant-specific file names in Step 4 breaks separation between configuration and implementation, reducing reusability and requiring code changes for environment variations.
-
Security Vulnerability: Empty authentication files may allow services to start in poorly-authenticated states, creating potential security risks rather than preventing them.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 3 comments | Edit Agent Settings • Read Docs
| AUTH_DIR="$CONFIG_DIR/objectstore/auths" | ||
| CCS_AUTH_DIR="$HOME/.ccs/cliproxy/auth" | ||
| DOTFILES_AUTH_DIR="$HOME/dotfiles/objectstore/auths" | ||
| REQUIRED_FILES=("shunkakinoki@gmail.com-shunkakinoki.json") |
There was a problem hiding this comment.
Hardcoding a user-specific email breaks the reusability of this script. Consider making REQUIRED_FILES configurable via an environment variable or external config file (e.g., ${REQUIRED_FILES:-"shunkakinoki@gmail.com-shunkakinoki.json"}) so the script can be used in different environments without code modifications.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#488
File: home-manager/services/cliproxyapi/scripts/backup-auth.sh#L13
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Hardcoding a user-specific email breaks the reusability of this script. Consider making REQUIRED_FILES configurable via an environment variable or external config file (e.g., `${REQUIRED_FILES:-"shunkakinoki@gmail.com-shunkakinoki.json"}`) so the script can be used in different environments without code modifications.
| else | ||
| echo "{}" >"$target" | ||
| chmod 600 "$target" | ||
| echo "⚠️ Created empty placeholder for $fname (no backup found)" >&2 |
There was a problem hiding this comment.
A warning message doesn't provide adequate visibility for missing critical authentication files. The script exits successfully (exit 0) even when required auth cannot be recovered, hiding bootstrap failures from monitoring systems. Consider exiting with a non-zero status or at minimum logging to a location that alerts can monitor. If placeholders are needed for local development, gate them behind an explicit flag.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#488
File: home-manager/services/cliproxyapi/scripts/backup-auth.sh#L59
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
A warning message doesn't provide adequate visibility for missing critical authentication files. The script exits successfully (exit 0) even when required auth cannot be recovered, hiding bootstrap failures from monitoring systems. Consider exiting with a non-zero status or at minimum logging to a location that alerts can monitor. If placeholders are needed for local development, gate them behind an explicit flag.
| @rsync@ -a "$DOTFILES_AUTH_DIR/$fname" "$target" | ||
| echo "✅ Restored missing $fname from dotfiles backup" >&2 | ||
| else | ||
| echo "{}" >"$target" |
There was a problem hiding this comment.
Creating an empty "{}" placeholder has critical implications: (1) This empty file will be synced back to R2 at line 66, potentially overwriting valid credentials in cloud storage and propagating the empty auth to all machines. (2) Services may start with invalid auth rather than failing visibly. Consider either: exiting with an error when required files can't be recovered, OR creating placeholders outside $AUTH_DIR so they never sync to R2, OR adding a flag to skip the R2 sync when placeholders were created.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#488
File: home-manager/services/cliproxyapi/scripts/backup-auth.sh#L57
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Creating an empty "{}" placeholder has critical implications: (1) This empty file will be synced back to R2 at line 66, potentially overwriting valid credentials in cloud storage and propagating the empty auth to all machines. (2) Services may start with invalid auth rather than failing visibly. Consider either: exiting with an error when required files can't be recovered, OR creating placeholders outside $AUTH_DIR so they never sync to R2, OR adding a flag to skip the R2 sync when placeholders were created.
Summary
Changes
Summary by cubic
Adds CCS auth directory sync on startup to include locally-created tokens and prevent bootstrap failures. Removes the required-files check from auth backup to simplify bootstrapping.
Written for commit ed3f7d5. Summary will update on new commits.