chore: add grok-cli dependency and improve backup services - #452
Conversation
…iles from dotfiles repo
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request increases backup and update frequencies across two services and enhances auth file synchronization logic. Darwin and Linux timer configurations are adjusted to run more frequently (every 3 minutes instead of 5 for cliproxyapi, and every 3 hours instead of daily for dotfiles-updater). The backup and recovery scripts add multi-step synchronization using AWS S3, including fallback mechanisms and redundancy paths. A new dependency is added to package.json. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~13 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
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 integrates a new AI-powered CLI tool, 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;DRAdd What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/cliproxyapi/scripts/recover-auth.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/recover-auth.sh:24">
P1: `aws s3 sync` returns success (exit code 0) even when the source is empty and nothing is synced. This means the backup fallback will be skipped even if no files were recovered from the main location. Consider also checking if files actually exist after sync:
```bash
if AWS_ACCESS_KEY_ID=... aws s3 sync ... && [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
```</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered auth files from backup" >&2 || echo "⚠️ Recovery failed (no backup available?)" >&2 | ||
| "$MAIN_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null; then |
There was a problem hiding this comment.
P1: aws s3 sync returns success (exit code 0) even when the source is empty and nothing is synced. This means the backup fallback will be skipped even if no files were recovered from the main location. Consider also checking if files actually exist after sync:
if AWS_ACCESS_KEY_ID=... aws s3 sync ... && [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; thenPrompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cliproxyapi/scripts/recover-auth.sh, line 24:
<comment>`aws s3 sync` returns success (exit code 0) even when the source is empty and nothing is synced. This means the backup fallback will be skipped even if no files were recovered from the main location. Consider also checking if files actually exist after sync:
```bash
if AWS_ACCESS_KEY_ID=... aws s3 sync ... && [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then
```</comment>
<file context>
@@ -1,22 +1,37 @@
- "$BACKUP_DIR" \
- "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered auth files from backup" >&2 || echo "⚠️ Recovery failed (no backup available?)" >&2
+ "$MAIN_DIR" \
+ "$AUTH_DIR/" 2>/dev/null; then
+ echo "✅ Recovered auth files from auths/" >&2
+ else
</file context>
| "$AUTH_DIR/" 2>/dev/null; then | |
| "$AUTH_DIR/" 2>/dev/null && [ -n "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if [ -d "$DOTFILES_AUTH_DIR" ] && [ -n "$(ls -A "$DOTFILES_AUTH_DIR" 2>/dev/null)" ]; then | ||
| mkdir -p "$AUTH_DIR" | ||
| rsync -a "$DOTFILES_AUTH_DIR/" "$AUTH_DIR/" |
There was a problem hiding this comment.
This new rsync call will fail under the Linux systemd unit because the service PATH is explicitly set to only bash/awscli2/coreutils (see home-manager/services/cliproxyapi/default.nix lines 106–112), so rsync is not available and set -e will terminate the backup/recovery cycle as soon as $HOME/dotfiles/objectstore/auths exists. That leaves auths unsynced and recovery skipped on systems where the dotfiles repo is present; either add pkgs.rsync to the PATH or gate this block on command -v rsync.
Useful? React with 👍 / 👎.
![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 introduces the @vibe-kit/grok-cli dependency, enhances the cliproxyapi backup system, and adjusts the dotfiles-updater schedule. The backup improvements are a good step towards robustness, adding primary and backup S3 locations and syncing from dotfiles. However, the shell scripts for backup and recovery use a brittle && ... || ... construct for error handling with set -e and suppress valuable error output from aws commands. I've provided suggestions to use standard if/else blocks and allow stderr to be logged for better reliability and debuggability. The other changes in the Nix configuration and package definitions are sound.
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "$MAIN_DIR" 2>/dev/null && echo "✅ Synced to auths/" >&2 || echo "⚠️ Sync to auths/ failed" >&2 |
There was a problem hiding this comment.
Using && ... || ... for control flow can be brittle, especially with set -e enabled. If the aws command fails, the script will exit, and the || part will not be executed. A standard if/else block is safer and more readable. Also, redirecting stderr to /dev/null hides potentially useful error messages. Since the service logs stderr, it's better to let aws errors be captured for easier debugging.
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "$MAIN_DIR" 2>/dev/null && echo "✅ Synced to auths/" >&2 || echo "⚠️ Sync to auths/ failed" >&2 | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "$MAIN_DIR"; then | |
| echo "✅ Synced to auths/" >&2 | |
| else | |
| echo "⚠️ Sync to auths/ failed" >&2 | |
| fi |
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "$BACKUP_DIR" 2>/dev/null || echo "⚠️ Backup failed (continuing anyway)" >&2 | ||
| "$BACKUP_DIR" 2>/dev/null && echo "✅ Synced to backup/auths/" >&2 || echo "⚠️ Backup sync failed" >&2 |
There was a problem hiding this comment.
This block has the same issues as the previous one: the && ... || ... construct is not robust with set -e, and redirecting stderr to /dev/null suppresses important error information. Refactoring to an if/else block is recommended for improved reliability and debuggability.
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "$BACKUP_DIR" 2>/dev/null || echo "⚠️ Backup failed (continuing anyway)" >&2 | |
| "$BACKUP_DIR" 2>/dev/null && echo "✅ Synced to backup/auths/" >&2 || echo "⚠️ Backup sync failed" >&2 | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "$BACKUP_DIR"; then | |
| echo "✅ Synced to backup/auths/" >&2 | |
| else | |
| echo "⚠️ Backup sync failed" >&2 | |
| fi |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered auth files from backup" >&2 || echo "⚠️ Recovery failed (no backup available?)" >&2 | ||
| "$MAIN_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null; then |
There was a problem hiding this comment.
Redirecting stderr to /dev/null hides potentially useful error messages from aws s3 sync. Since this script's stderr is captured by the service configuration, it's better to allow these errors to be logged for easier debugging.
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$BACKUP_DIR" \ | |
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered auth files from backup" >&2 || echo "⚠️ Recovery failed (no backup available?)" >&2 | |
| "$MAIN_DIR" \ | |
| "$AUTH_DIR/" 2>/dev/null; then | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$MAIN_DIR" \ | |
| "$AUTH_DIR/"; then |
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered from backup/auths/" >&2 || echo "⚠️ Recovery failed" >&2 |
There was a problem hiding this comment.
This block has the same issues as seen in backup-auth.sh: the && ... || ... construct is not robust with set -e, and redirecting stderr to /dev/null suppresses important error information. Refactoring to an if/else block is recommended for improved reliability and debuggability.
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$BACKUP_DIR" \ | |
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered from backup/auths/" >&2 || echo "⚠️ Recovery failed" >&2 | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| aws s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$BACKUP_DIR" \ | |
| "$AUTH_DIR/"; then | |
| echo "✅ Recovered from backup/auths/" >&2 | |
| else | |
| echo "⚠️ Recovery failed" >&2 | |
| fi |
There was a problem hiding this comment.
Pull request overview
This PR enhances the dotfiles repository by adding the @vibe-kit/grok-cli dependency for AI CLI capabilities and improving the cliproxyapi backup system with more frequent synchronization intervals and dual-location backup strategy. The changes also migrate the dotfiles-updater service from calendar-based to interval-based scheduling.
- Added @vibe-kit/grok-cli v0.0.34 dependency to package.json with proper trust configuration
- Improved cliproxyapi backup system with dual-location R2 sync (main + backup) and 3-minute intervals
- Modified dotfiles-updater to use 3-hour intervals with different timer configurations for macOS and Linux
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds @vibe-kit/grok-cli dependency and includes it in trustedDependencies list |
| bun.lock | Updates lockfile with new @vibe-kit/grok-cli package and all its transitive dependencies |
| home-manager/services/dotfiles-updater/default.nix | Changes from daily calendar-based runs to 3-hour interval-based scheduling for both macOS and Linux |
| home-manager/services/cliproxyapi/scripts/recover-auth.sh | Implements two-tier recovery strategy, attempting main location first then falling back to backup |
| home-manager/services/cliproxyapi/scripts/backup-auth.sh | Adds dotfiles repo syncing and dual-location backup to both main and backup R2 paths |
| home-manager/services/cliproxyapi/default.nix | Reduces backup interval from 5 minutes to 3 minutes for more frequent synchronization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
| Timer = { | ||
| OnCalendar = "*-*-* 00:00:00"; | ||
| OnCalendar = "*-*-* 00/3:00:00"; |
There was a problem hiding this comment.
The OnCalendar value "--* 00/3:00:00" is incorrectly formatted for systemd timers. The "/3" notation in the hour field means "every 3 hours starting from hour 00", but the minute and second fields should not both be "00:00".
For a timer that runs every 3 hours, the correct format should be:
- "--* *:00:00" with OnUnitActiveSec = "3h" (preferred for recurring tasks)
- OR "--* 00/3:00:00" but this is non-standard syntax
To match the macOS StartInterval of 10800 seconds (3 hours), consider using OnUnitActiveSec instead of OnCalendar for consistency.
| OnCalendar = "*-*-* 00/3:00:00"; | |
| OnUnitActiveSec = "3h"; |
| # First, sync from dotfiles repo to local cache (picks up new auth files from ccs auth) | ||
| if [ -d "$DOTFILES_AUTH_DIR" ] && [ -n "$(ls -A "$DOTFILES_AUTH_DIR" 2>/dev/null)" ]; then | ||
| mkdir -p "$AUTH_DIR" | ||
| rsync -a "$DOTFILES_AUTH_DIR/" "$AUTH_DIR/" |
There was a problem hiding this comment.
Using rsync -a without error handling could silently fail. If rsync fails (e.g., due to permissions or I/O errors), the script will continue with potentially incomplete or corrupted auth files being synced to R2. Consider adding error handling similar to the aws s3 sync commands, or use set -e behavior by checking the rsync exit code.
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "$MAIN_DIR" 2>/dev/null && echo "✅ Synced to auths/" >&2 || echo "⚠️ Sync to auths/ failed" >&2 | ||
|
|
||
| # Also sync to backup location for redundancy | ||
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "$BACKUP_DIR" 2>/dev/null || echo "⚠️ Backup failed (continuing anyway)" >&2 | ||
| "$BACKUP_DIR" 2>/dev/null && echo "✅ Synced to backup/auths/" >&2 || echo "⚠️ Backup sync failed" >&2 |
There was a problem hiding this comment.
The error handling pattern with || echo at the end of each aws s3 sync command prevents the script from failing (via set -e) even when sync operations fail. If both the main and backup sync operations fail, the script will exit successfully despite no files being uploaded. Consider tracking failures and exiting with a non-zero status code if critical operations fail, especially for the main sync to auths/.
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered auth files from backup" >&2 || echo "⚠️ Recovery failed (no backup available?)" >&2 | ||
| "$MAIN_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null; then | ||
| echo "✅ Recovered auth files from auths/" >&2 | ||
| else | ||
| # Fall back to backup location | ||
| echo "Main location empty, trying backup..." >&2 | ||
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| aws s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered from backup/auths/" >&2 || echo "⚠️ Recovery failed" >&2 |
There was a problem hiding this comment.
The recovery script suppresses all stderr output from aws s3 sync with 2>/dev/null, making it difficult to diagnose failures. When the main location sync fails, it's unclear whether it failed because the location is empty, network issues occurred, or authentication failed. Consider capturing and logging the error output to help with troubleshooting, or at least distinguishing between "no files found" and "sync operation failed".
| --no-progress \ | ||
| "$BACKUP_DIR" \ | ||
| "$AUTH_DIR/" 2>/dev/null && echo "✅ Recovered from backup/auths/" >&2 || echo "⚠️ Recovery failed" >&2 | ||
| fi |
There was a problem hiding this comment.
After recovery attempts, there's no verification that files were actually recovered. The script could complete successfully even if both sync operations fail or return no files. Consider adding a check after the recovery attempts to verify that $AUTH_DIR is not empty, and exit with an error if recovery failed to restore any files.
| fi | |
| fi | |
| # Verify that recovery actually restored at least one auth file | |
| if [ -z "$(ls -A "$AUTH_DIR" 2>/dev/null)" ]; then | |
| echo "❌ Recovery failed: no auth files present in $AUTH_DIR after recovery attempts" >&2 | |
| exit 1 | |
| fi |
| "${backupScripts}/backup-and-recover.sh" | ||
| ]; | ||
| StartInterval = 300; # Run every 5 minutes | ||
| StartInterval = 180; # Run every 3 minutes |
There was a problem hiding this comment.
The backup interval was reduced from 5 minutes (300 seconds) to 3 minutes (180 seconds). This 40% increase in frequency may put additional load on the R2 storage service with more frequent sync operations. Consider whether this increased frequency is necessary, especially since the cliproxyapi service appears to use KeepAlive=true (continuously running), so auth files should rarely be missing. A less aggressive interval might be sufficient.
Summary
Summary by cubic
Adds @vibe-kit/grok-cli and hardens cliproxyapi auth backup/recovery by syncing from dotfiles and pushing to both primary and backup. Also reduces backup cadence to 3 minutes and runs dotfiles-updater every 3 hours.
Dependencies
Bug Fixes
Written for commit 528c033. Summary will update automatically on new commits.